Use code splitting for default and no-important builds#325
Merged
Conversation
If two copies of Aphrodite end up being run at the same time, you can easily encounter issues due to state stored by Aphrodite not being shared between the multiple copies (e.g. `isBuffering`). Since v2, when we started using rollup to build Aphrodite, this was pretty easy to accidentally do if you weren't careful about when you imported from `aphrodite` or `aphrodite/no-important`. This has resulted in some issues, particularly around react-with-styles-interface-amp-aphrodite and hypernova-amp not being super careful about which version of aphrodite they import. One of the recent fixes for this can be seen here: airbnb/react-with-styles-interface-amp-aphrodite#8 In an effort to make this safer, I am making a small change to how we build this library. Rollup offers an experimental code splitting mode that allows us to keep the separate entry points and automatically dedupe all of the shared code into a single chunk that each entry point imports. As a result, we can provide the same API we currently do to consumers, and avoid this issue to some extent. The result of running this build on the es directory looks like the following: - chunk-957f2f88.js - index.js - no-important.js and es/index.js ends up looking like: ```js import { a as makeExports } from './chunk-957f2f88.js'; import 'string-hash'; import 'inline-style-prefixer/static/plugins/calc'; import 'inline-style-prefixer/static/plugins/crossFade'; import 'inline-style-prefixer/static/plugins/cursor'; import 'inline-style-prefixer/static/plugins/filter'; import 'inline-style-prefixer/static/plugins/flex'; import 'inline-style-prefixer/static/plugins/flexboxIE'; import 'inline-style-prefixer/static/plugins/flexboxOld'; import 'inline-style-prefixer/static/plugins/gradient'; import 'inline-style-prefixer/static/plugins/imageSet'; import 'inline-style-prefixer/static/plugins/position'; import 'inline-style-prefixer/static/plugins/sizing'; import 'inline-style-prefixer/static/plugins/transition'; import 'inline-style-prefixer/static/createPrefixer'; import 'asap'; var useImportant = true; // Add !important to all style definitions var Aphrodite = makeExports(useImportant); var StyleSheet = Aphrodite.StyleSheet, StyleSheetServer = Aphrodite.StyleSheetServer, StyleSheetTestUtils = Aphrodite.StyleSheetTestUtils, css = Aphrodite.css, minify = Aphrodite.minify, flushToStyleTag = Aphrodite.flushToStyleTag, injectAndGetClassName = Aphrodite.injectAndGetClassName, defaultSelectorHandlers = Aphrodite.defaultSelectorHandlers; export { StyleSheet, StyleSheetServer, StyleSheetTestUtils, css, minify, flushToStyleTag, injectAndGetClassName, defaultSelectorHandlers }; ``` (es/no-important.js looks almost identical) The bulk of the library ends up in es/chunk-957f2f88.js, which should allow the state to be consolidated in this scenario.
Collaborator
Author
|
@xymostech I believe this unbreaks something for us, so I'm planning on getting this released ASAP. Please take a look soon if you have a moment. |
Collaborator
Author
ljharb
reviewed
May 11, 2018
ljharb
left a comment
There was a problem hiding this comment.
This LGTM, but if the chunk filename (specifically, the "957f2f88") changes, I'd consider that a semver-major change. Is there a way to hardcode the name of the chunk file, or make it deterministic?
Collaborator
Author
|
The chunk filename is a hash of its contents. I don't think it is configurable. Here's the documentation: https://rollupjs.org/guide/en#experimental-options I think it is acceptable to consider this not part of the public API and not consider any changes to this filename as a breaking change. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If two copies of Aphrodite end up being run at the same time, you can
easily encounter issues due to state stored by Aphrodite not being
shared between the multiple copies (e.g.
isBuffering).Since v2, when we started using rollup to build Aphrodite, this was
pretty easy to accidentally do if you weren't careful about when you
imported from
aphroditeoraphrodite/no-important. This has resultedin some issues, particularly around
react-with-styles-interface-amp-aphrodite and hypernova-amp not being
super careful about which version of aphrodite they import. One of the
recent fixes for this can be seen here:
airbnb/react-with-styles-interface-amp-aphrodite#8
In an effort to make this safer, I am making a small change to how we
build this library. Rollup offers an experimental code splitting mode
that allows us to keep the separate entry points and automatically
dedupe all of the shared code into a single chunk that each entry point
imports. As a result, we can provide the same API we currently do to
consumers, and avoid this issue to some extent.
The result of running this build on the es directory looks like the
following:
and es/index.js ends up looking like:
(es/no-important.js looks almost identical)
The bulk of the library ends up in es/chunk-957f2f88.js, which should
allow the state to be consolidated in this scenario.