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

Add /esm build #791

Merged
merged 2 commits into from Oct 24, 2017

Conversation

Projects
None yet
4 participants
@lencioni
Member

lencioni commented Oct 23, 2017

This ES modules build will make these modules smaller in the final
bundles, since the way Babel compiles import to require is pretty
verbose. Additionally, it will enable webpack to make some optimizations
on the final bundle, including tree-shaking and scope hoisting (if
enabled). These will improve bundle sizes and runtime performance.

I considered adding a module field to the package.json to point to this
version for folks who are able to use it, but I decided to wait on that
for now since that is still pretty experimental.

For now, consumers will need to use this either by importing from
react-dates/esm directly or by using something like a webpack alias to
use this version of the build.

When working on this I originally kept the plugins array at the top
level instead of repeating it for each env, but I ran into an issue that
seemed related to ordering that caused babel-register in the compile CSS
script to not apply the correct transformations. I was able to work
around this by repeating the plugins for each env.

~/react-dates ❯❯❯ du -hs lib
508K    lib
~/react-dates ❯❯❯ du -hs esm
472K    esm

@airbnb/webinfra

@lencioni lencioni requested review from ljharb and majapw Oct 23, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 23, 2017

Coverage Status

Coverage decreased (-86.6%) to 0.07% when pulling 5424418 on jdl-esm-build into a865f5b on master.

coveralls commented Oct 23, 2017

Coverage Status

Coverage decreased (-86.6%) to 0.07% when pulling 5424418 on jdl-esm-build into a865f5b on master.

lencioni added some commits Oct 23, 2017

Build entry points
These three files are used as entry points into the project, and we
expect people to be importing them directly so they are at the top level
for convenience. Because of this, they have not been getting run through
Babel like the rest of the code here, so they were using older syntax.

In this commit, I am moving these files into the src directory so they
will be built into lib like the rest of the code here. In their place at
the top level, I am creating some very light shims that simply bring in
the built version of the module and re-export it if necessary.

This gives us a couple of advantages. First, it makes more of our code
consistent. Second, it makes it possible to produce two different kinds
of builds, one for CommonJS consumers and another for ES modules
consumers.
Add /esm build
This ES modules build will make these modules smaller in the final
bundles, since the way Babel compiles import to require is pretty
verbose. Additionally, it will enable webpack to make some optimizations
on the final bundle, including tree-shaking and scope hoisting (if
enabled). These will improve bundle sizes and runtime performance.

I considered adding a module field to the package.json to point to this
version for folks who are able to use it, but I decided to wait on that
for now since that is still pretty experimental.

For now, consumers will need to use this either by importing from
react-dates/esm directly or by using something like a webpack alias to
use this version of the build.

When working on this I originally kept the plugins array at the top
level instead of repeating it for each env, but I ran into an issue that
seemed related to ordering that caused babel-register in the compile CSS
script to not apply the correct transformations. I was able to work
around this by repeating the plugins for each env.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 23, 2017

Coverage Status

Coverage increased (+0.1%) to 86.76% when pulling 2124613 on jdl-esm-build into a865f5b on master.

coveralls commented Oct 23, 2017

Coverage Status

Coverage increased (+0.1%) to 86.76% when pulling 2124613 on jdl-esm-build into a865f5b on master.

@ljharb

ljharb approved these changes Oct 23, 2017

LGTM

toLocalizedDateString: toLocalizedDateString,
toMomentObject: toMomentObject,
};
// eslint-disable-next-line import/no-unresolved

This comment has been minimized.

@ljharb

ljharb Oct 23, 2017

Member

i don't think this is needed; it's fine if the linter requires the build process be ran first.

@ljharb

ljharb Oct 23, 2017

Member

i don't think this is needed; it's fine if the linter requires the build process be ran first.

This comment has been minimized.

@lencioni

lencioni Oct 23, 2017

Member

I mostly wanted to get rid of the error messages for myself when I was working in these files.

@lencioni

lencioni Oct 23, 2017

Member

I mostly wanted to get rid of the error messages for myself when I was working in these files.

This comment has been minimized.

@ljharb

ljharb Oct 23, 2017

Member

npm run build would have done that :-p

@ljharb

ljharb Oct 23, 2017

Member

npm run build would have done that :-p

This comment has been minimized.

@lencioni

lencioni Oct 24, 2017

Member

And then npm run clean would have broken it again :-P

@lencioni

lencioni Oct 24, 2017

Member

And then npm run clean would have broken it again :-P

This comment has been minimized.

@ljharb

ljharb Oct 24, 2017

Member

It’s not a blocker; but in general I think the warnings there are good; without them, you might unintentionally have a bad path. I don’t think it’s a good idea to optimize for the development case when it subtracts from production safety ¯\_(ツ)_/¯

@ljharb

ljharb Oct 24, 2017

Member

It’s not a blocker; but in general I think the warnings there are good; without them, you might unintentionally have a bad path. I don’t think it’s a good idea to optimize for the development case when it subtracts from production safety ¯\_(ツ)_/¯

@@ -1,4 +1,2 @@
// eslint-disable-next-line import/no-unresolved

This comment has been minimized.

@ljharb

ljharb Oct 23, 2017

Member

same here

@ljharb

ljharb Oct 23, 2017

Member

same here

}
"presets": ["airbnb"],
"plugins": [
"inline-react-svg",

This comment has been minimized.

@ljharb

ljharb Oct 23, 2017

Member

super gnarly that we have to repeat these two plugins in every env due to ordering issues

@ljharb

ljharb Oct 23, 2017

Member

super gnarly that we have to repeat these two plugins in every env due to ordering issues

@majapw

majapw approved these changes Oct 24, 2017

LGTM!

@lencioni lencioni merged commit ef98109 into master Oct 24, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 86.76%
Details

@lencioni lencioni deleted the jdl-esm-build branch Oct 24, 2017

@majapw

This comment has been minimized.

Show comment
Hide comment
@majapw

majapw Oct 27, 2017

Contributor

@lencioni @ljharb I think this change affected the gh-pages script which builds storybook. Specifically, I think it is using a babel environment that has no associated setting in the .babelrc.

Here's the error I'm getting:

Module build failed: SyntaxError: Unexpected token (32:2)

  30 | 
  31 | addDecorator(story => (
> 32 |   <div>
     |   ^
  33 |     <div
  34 |       style={{
  35 |         background: '#fff',

BabelLoaderError: SyntaxError: Unexpected token (32:2)

  30 | 
  31 | addDecorator(story => (
> 32 |   <div>
     |   ^
  33 |     <div
  34 |       style={{
  35 |         background: '#fff',

    at transpile (/Users/maja_wichrowska/code/airlab/repos/react-dates/node_modules/@storybook/react/node_modules/babel-loader/lib/index.js:65:13)
    at /Users/maja_wichrowska/code/airlab/repos/react-dates/node_modules/@storybook/react/node_modules/babel-loader/lib/fs-cache.js:118:18
    at ReadFileContext.callback (/Users/maja_wichrowska/code/airlab/repos/react-dates/node_modules/@storybook/react/node_modules/babel-loader/lib/fs-cache.js:31:21)
    at FSReqWrap.readFileAfterOpen [as oncomplete] (fs.js:419:13)
 @ multi ./~/@storybook/react/dist/server/config/polyfills.js ./~/@storybook/react/dist/server/config/globals.js ./.storybook/config.js
Contributor

majapw commented Oct 27, 2017

@lencioni @ljharb I think this change affected the gh-pages script which builds storybook. Specifically, I think it is using a babel environment that has no associated setting in the .babelrc.

Here's the error I'm getting:

Module build failed: SyntaxError: Unexpected token (32:2)

  30 | 
  31 | addDecorator(story => (
> 32 |   <div>
     |   ^
  33 |     <div
  34 |       style={{
  35 |         background: '#fff',

BabelLoaderError: SyntaxError: Unexpected token (32:2)

  30 | 
  31 | addDecorator(story => (
> 32 |   <div>
     |   ^
  33 |     <div
  34 |       style={{
  35 |         background: '#fff',

    at transpile (/Users/maja_wichrowska/code/airlab/repos/react-dates/node_modules/@storybook/react/node_modules/babel-loader/lib/index.js:65:13)
    at /Users/maja_wichrowska/code/airlab/repos/react-dates/node_modules/@storybook/react/node_modules/babel-loader/lib/fs-cache.js:118:18
    at ReadFileContext.callback (/Users/maja_wichrowska/code/airlab/repos/react-dates/node_modules/@storybook/react/node_modules/babel-loader/lib/fs-cache.js:31:21)
    at FSReqWrap.readFileAfterOpen [as oncomplete] (fs.js:419:13)
 @ multi ./~/@storybook/react/dist/server/config/polyfills.js ./~/@storybook/react/dist/server/config/globals.js ./.storybook/config.js
@lencioni

This comment has been minimized.

Show comment
Hide comment
@lencioni

lencioni Oct 27, 2017

Member
Member

lencioni commented Oct 27, 2017

@majapw

This comment has been minimized.

Show comment
Hide comment
@majapw

majapw Oct 27, 2017

Contributor

I think it builds in production mode, but I'm unsure.

EDIT: yup, it looks like it does
https://github.com/storybooks/storybook/blob/master/app/react/src/server/build.js#L14

Do we have to add another setting for that?

Contributor

majapw commented Oct 27, 2017

I think it builds in production mode, but I'm unsure.

EDIT: yup, it looks like it does
https://github.com/storybooks/storybook/blob/master/app/react/src/server/build.js#L14

Do we have to add another setting for that?

lencioni added a commit that referenced this pull request Oct 27, 2017

Add Babel config for production environment
In #791 I added an ESM build,
but ended up breaking the production build for gh-pages. Storybook
builds with NODE_ENV=production, so this should fix that problem.

@lencioni lencioni referenced this pull request Dec 1, 2017

Open

Webpack bundle.js size #874

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment