Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move to TypeScript, use Rollup to bundle, inline SVGs, export constants rather than members #181

Closed
wants to merge 11 commits into from

Conversation

osdiab
Copy link
Contributor

@osdiab osdiab commented May 10, 2017

Jira:

  • Breaking change - export constants instead of members on component functions and classes. FEE-74
  • Use TypeScript instead of JavaScript FEE-78
  • Use Rollup as the bundler, to allow for code splitting CLASSROOMS-1493
  • Inline SVGs FEE-76

TODO:

  • decide if the breaking change of constant exports instead of members is OK
  • decide if we actually want to use Rollup, or wait for the below webpack issue to be completed / work on it ourselves

If using Rollup,

  • Integrate it with the dev server
  • Remove webpack stuff

If not using Rollup,

  • Remove the rollup code and dependencies
  • Setup the TypeScript compiler to properly build our code (would have to disable typechecking, I believe)

Overview:
Initially started as an attempt to use react-svg-loader with webpack to allow import of SVGs. Turns out that we don't use webpack in clever-components since it doesn't output a code-splittable/tree-shakeable bundle.

A code-splittable bundle needs to use ES6 exports so that bundling tools like Webpack 2 or Rollup can do the dead code elimination on the dependency graph. Webpack 2 supports tree-shaking, but in the bundle it outputs, it currently doesn't support producing ES6-export-style code with the libraryTarget compiler option. Rollup, however does.

So, this produces bundles for pkg.main for non-es6-capable clients, and pkg.module for es6-import-aware clients. I introduced TypeScript while doing that, and because all of the members on the components and functions were throwing type errors, I moved those to constants.

Unfortunately, I ended up not using an svg to React preprocessor for the original task of inlining of SVGs, for two reasons:

  • There is not a rollup plugin yet that does that (although I am confident with some time I could make one!).
  • TypeScript throws errors because it cannot infer the output types from the SVGs; so for the TypeScript type checker to be happy while in the dev environment, including SVG's directly would require setting a type on every SVG export. This is a little ugly, but not the worst thing.

Instead, they have been converted into React components.

Testing:

  • Unit tests
  • Manual tests:
    • Chrome
    • Safari
    • IE10

Roll Out:

  • Before merging:
    • Updated docs
    • Bumped version in package.json
  • After merging:
    • Deployed updated docs (make deploy-docs)

@osdiab osdiab requested a review from kofi-clever May 10, 2017 18:33
@osdiab osdiab self-assigned this May 10, 2017
@kofi-clever
Copy link
Contributor

high-level comment on the constants:
doing a similar js[x] -> ts[x] migration in apps-dashboard and feel like we can still preserve that behaviour.
instead of:

class Component {
  render() {}
}

Component.Size = {
  SMALL: "small",
  MEDIUM: "medium",
};

we'd do:

class Component {
  static Size = {
    SMALL: "small",
    MEDIUM: "medium",
  }; 

  render() {}
}

also curious about the naming changes. thought it was common practice to name enums in the singular (although granted, js doesn't natively have enums in the traditional sense)

@kofi-clever
Copy link
Contributor

also (if it's not already the plan) we can keep this as a prototyping branch and break off the various pieces to address one at a time.

in particular, not fully convinced yet that we have problems that need solving with rollup. i might even sooner go with a build task manager like gulp or something if we start needing for pre-publish transformations.

title: PropTypes.string.isRequired,
type: PropTypes.oneOf(_.values(TYPES)),
isClosable: PropTypes.bool,
children: React.PropTypes.node.isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we're making these changes anyway, we might as well migrate our PropType references to import * as PropTypes from "prop-types";

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice, didn't realize they split it out :)

@@ -62,9 +62,9 @@ const TYPES = {
};

AlertBox.propTypes = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would like to see us move these to truly static fields, e.g.:

class AlertBox extends Component {
  static propTypes = {
    // ...
  };

  render() { /* ... */ }
}

so that our components are properly statically typed.
then we can remove the hack we have in the minimal typings.
what i'm leaning towards as part of the apps-dashboard conversion work: https://github.com/Clever/apps-dashboard/compare/tsx#diff-68c212eb090c15f35ca18fac07925830

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, why not include the modifications to apps dashboard react typings to the minimal-react-typings repo?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, planning on that, but it'll be a breaking change, so wanted to prototype a bit before merging there

@@ -1,6 +1,6 @@
import _ from "lodash";
import classnames from "classnames";
import React, {PureComponent, PropTypes} from "react";
import * as React from "react";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the others also need to be import * as _ from "lodash"; and import * as classnames from "classnames";

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, I actually couldn't do import * as classnames because of a limitation of Rollup/ES6
Modules—tools like Webpack and TypeScript, for backwards compatibility with old node-style exports, interprets import * calls for non-ES6-module code as a way to access the module.exports variable. But in pure ES6 semantics, import * actually returns a namespace object, which is not invocable. So, Rollup complains.

It works here because I set in tsconfig.json to allow the synthetic module imports, so that import classnames gets interpreted as importing module.exports; however, React actually is being exported properly, so it uses actual ES6 semantics, which calls import React an incorrect usage.

This is further complicated since Babel is even more lenient. It will all you to refer to default exports via import classnames as though module.exports were a default export, whereas TypeScript requires the synthetic imports flag to make that happen.

rollup/rollup#670

@osdiab
Copy link
Contributor Author

osdiab commented May 10, 2017

As far as the enum naming, I was thinking of them more of a list of valid values as opposed to an enum, since as you mentioned JS doesn't have enums. If we were expressing them as a TypeScript union type or something, I'd agree, but we're not, so I think this is more accurate, especially in the parts of our code where we iterate over them or do other collection operations on the valid values.

To retain the backwards compatibility though we can use the static properties, though I do think just exporting it is a better long term choice, as it achieves the same effect, allows importing the constants selectively, and doesn't have any potential namespace clashes with future React functionality, bringing us closer to a world where we can use the actual @types/react rather than minimal-react-typings

@osdiab osdiab closed this Jun 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants