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

Revert 3260 #3346

Merged
merged 3 commits into from
Jan 16, 2019
Merged

Revert 3260 #3346

merged 3 commits into from
Jan 16, 2019

Conversation

gwyneplaine
Copy link
Collaborator

@gwyneplaine gwyneplaine commented Jan 14, 2019

Reasoning

The introduction of #3260 as part of v2.2.0 makes SSR impossible, as a custom instance of emotion is being created and used. See #3317 for details on the issue. This PR aims to revert this feature, in order to resolve this issue within the 2.x stream of work.

Users depending on nonces for CSP support should be advised to pin their dependency at 2.2.x, to continue to leverage this feature.

In addition there is a body of work to update react-select to use emotion 10. Which reintroduces nonce support in the form of a NonceProvider component. However this is slated as a breaking change, and will be released as part of 3.x.x. See #3321 for details

Summary

This PR encapsulates the following changes:

  • remove create-emotion as a dependency
  • remove emotion from the internal api of custom components (inclusive of ScrollBlock and A11yText)
  • remove the nonce prop
  • re-add emotion 9.2.0 as a dependency

@idan
Copy link

idan commented Jan 15, 2019

There's another major casualty brought on by the switch to passing emotion instances as props: snapshot testing.

We recently attempted a bump of our (large-ish) codebase from 2.1.1 to 2.2.1, and discovered that the instance-passing led to massive inflation of Jest snapshot tests:

screen shot 2019-01-15 at 09 23 51

In our experience, Jest tends to hang with overly large snapshot files. #3260 made many of our snapshots balloon in size, causing some tests to hang.

I don't have an informed opinion about the overall approach, but some consideration / affordances to make testing sane and safe would be a welcome step before reintroducing the emotion-instance-as-prop approach. Or maybe there's already a best-practice here that we're missing? 😢

@gwyneplaine
Copy link
Collaborator Author

@idan we wouldn't be reintroducing emotion as a prop in 3.0.0, upgrading to emotion 10 affords us a way to pass nonce values into the emotions cache without having to create a custom emotion instance. Sorry to hear about the jest tests trouble

@idan
Copy link

idan commented Jan 16, 2019

🎉 Awesome to hear!

I'd like to politely request a release if it's possible to roll one soon; we have an interaction bug with <Select>s that is fixed in the new release but the emotion/jest shenanigans are blocking us — now that this is reverted I'd love to be able to update.

Thank you!

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.

2 participants