Avoid using two copies of Aphrodite for no-important by lencioni · Pull Request #8 · airbnb/react-with-styles-interface-amp-aphrodite

lencioni · 2018-05-11T00:28:21Z

After updating to v2 of this package, we noticed that our AMP builds
started failing with errors like

Cannot automatically buffer without a document

After some investigation I believe I understand why this happened. The
main difference is that Aphrodite v2 is built using rollup, which means
that the default import and the no-important import are two entirely
separate self-contained modules. And since these modules maintain state,
it is important for them to be the same.

To fix this, I am moving the importing of aphrodite or
aphrodite/no-important up to the entry point level of this package,
and passing down the useful parts to be consumed by the factory. This
ensures that the same version of Aphrodite is used throughout.

This should have been caught by tests if we hadn't suppressed style
injection, which we don't need to do in this package because of how we
interact with Aphrodite, and if we were using the resolve method and
setting the AMP environment variable. I've addressed all of these issues
in this PR.

While testing this I noticed that when the AMP environment variable is
set, it never uses !important even if the non AMP styles use
!important. I think this is probably fine due to the constraints of
AMP, but I wasn't entirely sure if it was intentional. I added some
tests to showcase this behavior.

We should follow this up with work in Aphrodite itself that makes this
safer.

Add fallback for factory old behavior

In fixing the critical bug that shipped with v2, I needed to change the
API of this function, which makes the bugfix a semver major change. To
make this a semver-minor change, I am adding a fallback here so it still
works as it used to (in a buggy way) if this function is called directly
by some unknown outside consumer.

I think we could potentially detect whether to import from aphrodite or
aphrodite/no-important based on the shape of aphroditeInterface here,
but since I don't expect anyone to actually be hitting this codepath, I
didn't bother. The console warning should be enough to help folks
resolve this issue without too much work.

ljharb · 2018-05-11T00:29:40Z

@@ -1,9 +1,3 @@
-import {


this change makes it semver-major; if we keep these and use the passed args as overrides when present, then it'd be semver-minor

Ok, I'll do that

After updating to v2 of this package, we noticed that our AMP builds started failing with errors like > Cannot automatically buffer without a document After some investigation I believe I understand why this happened. The main difference is that Aphrodite v2 is built using rollup, which means that the default import and the no-important import are two entirely separate self-contained modules. And since these modules maintain state, it is important for them to be the same. To fix this, I am moving the importing of `aphrodite` or `aphrodite/no-important` up to the entry point level of this package, and passing down the useful parts to be consumed by the factory. This ensures that the same version of Aphrodite is used throughout. This should have been caught by tests if we hadn't suppressed style injection, which we don't need to do in this package because of how we interact with Aphrodite, and if we were using the resolve method and setting the AMP environment variable. I've addressed all of these issues in this PR. While testing this I noticed that when the AMP environment variable is set, it never uses `!important` even if the non AMP styles use `!important`. I think this is probably fine due to the constraints of AMP, but I wasn't entirely sure if it was intentional. I added some tests to showcase this behavior. We should follow this up with work in Aphrodite itself that makes this safer.

majapw

Huh, there was a point where we were definitely stripping out !important flags in the Amp environment (they aren't allowed) and I seem to have lost that code. :/ But I don't think you changed that so now I'm just confused.

In any case, the code looks good! :) what's up with the tests tho?

cc: @gilbox

In fixing the critical bug that shipped with v2, I needed to change the API of this function, which makes the bugfix a semver major change. To make this a semver-minor change, I am adding a fallback here so it still works as it used to (in a buggy way) if this function is called directly by some unknown outside consumer. I think we could potentially detect whether to import from aphrodite or aphrodite/no-important based on the shape of `aphroditeInterface` here, but since I don't expect anyone to actually be hitting this codepath, I didn't bother. The console warning should be enough to help folks resolve this issue without too much work.

lencioni · 2018-05-11T15:51:53Z

@majapw I updated my PR and I think I found that code. PTAL

ljharb · 2018-05-11T16:14:47Z

+  // breaking them.
+  // TODO: Remove this block in next semver-major change
+  if (!injectAndGetClassName || !defaultSelectorHandlers || !flushToStyleTag) {
+    console.warn('You appear to be using ampAphroditeInterfaceFactory in a deprecated a buggy way. Please pass in `injectAndGetClassName`, `defaultSelectorHandlers`, and `flushToStyleTag` as arguments to this function.');


I don't believe anyone will ever see this message anyway

lencioni · 2018-05-11T16:25:39Z

This was a good change to land, but unfortunately, it doesn't resolve the entire issue for us because of this line in hypernova-amp: https://github.com/airbnb/hypernova-amp/blob/a38459215489db707dfa847662e8f0c6a081b66d/src/createAmpRender.js#L3

We can probably address that in hypernova-amp by structuring it similarly to how we have structured this project. But I also think I can make some changes to how aphrodite is built to make it safer.

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.

lencioni requested review from ljharb and majapw May 11, 2018 00:28

ljharb approved these changes May 11, 2018

View reviewed changes

majapw approved these changes May 11, 2018

View reviewed changes

lencioni force-pushed the fix-no-important branch from e840b5b to 41a3445 Compare May 11, 2018 15:51

lencioni merged commit 4a9ada3 into master May 11, 2018

lencioni deleted the fix-no-important branch May 11, 2018 16:00

ljharb reviewed May 11, 2018

View reviewed changes

lencioni mentioned this pull request May 11, 2018

Use code splitting for default and no-important builds Khan/aphrodite#325

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid using two copies of Aphrodite for no-important#8

Avoid using two copies of Aphrodite for no-important#8
lencioni merged 2 commits into
masterfrom
fix-no-important

lencioni commented May 11, 2018 •

edited

Loading

Uh oh!

ljharb May 11, 2018

Uh oh!

lencioni May 11, 2018

Uh oh!

majapw left a comment •

edited

Loading

Uh oh!

lencioni commented May 11, 2018

Uh oh!

ljharb May 11, 2018

Uh oh!

lencioni May 11, 2018

Uh oh!

lencioni May 11, 2018

Uh oh!

lencioni commented May 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

lencioni commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb May 11, 2018

Choose a reason for hiding this comment

Uh oh!

lencioni May 11, 2018

Choose a reason for hiding this comment

Uh oh!

majapw left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lencioni commented May 11, 2018

Uh oh!

ljharb May 11, 2018

Choose a reason for hiding this comment

Uh oh!

lencioni May 11, 2018

Choose a reason for hiding this comment

Uh oh!

lencioni May 11, 2018

Choose a reason for hiding this comment

Uh oh!

lencioni commented May 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lencioni commented May 11, 2018 •

edited

Loading

majapw left a comment •

edited

Loading