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

Upgrades Emotion to v11 #4283

Merged
merged 11 commits into from
Jan 22, 2021
Merged

Conversation

majgaard
Copy link
Contributor

@majgaard majgaard commented Nov 15, 2020

Hi 👋

With Emotion v11 being released and many other packages (including Chakra-UI) upgrading their dependencies, react-select should follow suit.

The predominant reason for this is that having mismatched versions of Emotion can cause lots of issues. See this issue for one of many examples.

@changeset-bot
Copy link

changeset-bot bot commented Nov 15, 2020

🦋 Changeset detected

Latest commit: e5151bb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@react-select/docs Major
react-select Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 15, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit e5151bb:

Sandbox Source
react-codesandboxer-example Configuration

babel.config.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@giovannigiordano
Copy link

@belgattitude @majgaard any update on this?

@majgaard
Copy link
Contributor Author

majgaard commented Dec 3, 2020

This is largely good-to-go from my end. There are 2 Flow-typing issues. One can be fixed somewhat easily, but I'm not sure how the maintainers would prefer to fix it. The other issue is because Flow can't find the Stylis module. I'm not particularly familiar with Flow and I have been unable to fix the issue.

@Methuselah96 Methuselah96 added this to the 4.0 milestone Dec 12, 2020
@sneridagh
Copy link

@majgaard @JedWatson This is starting to be painful, any update? Sorry, but I can't do Flow, so I'm not able to jump in.

@Methuselah96
Copy link
Collaborator

Methuselah96 commented Dec 17, 2020

@sneridagh We're planning to include this in 4.0 (which we're hoping to release soon). I'm looking into the Flow errors now.

@with-heart
Copy link

Hi @Methuselah96, Chakra UI maintainer here. Glad to hear this is scheduled for v4 🎉 Let me know if there's anything I can do to assist with this!

yarn.lock Outdated Show resolved Hide resolved
Copy link
Owner

@JedWatson JedWatson left a comment

Choose a reason for hiding this comment

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

Looks good, two small changes and we can get this merged

packages/react-select/src/NonceProvider.js Outdated Show resolved Hide resolved
.changeset/olive-toys-try.md Outdated Show resolved Hide resolved
@majgaard
Copy link
Contributor Author

We should be all good! 🎉

@kpunith8
Copy link

@majgaard looks like @emotion/jest is not updated to the latest version, it is still pointing to jest-emotion, and is used here, https://github.com/JedWatson/react-select/blob/master/packages/react-select/src/__tests__/Select.test.js#L15 I found this while I was forking and updating to emotion-11 using the codemod rule "@emotion/pkg-renaming": "error"

Not sure this is a blocker for this PR to go.

@majgaard
Copy link
Contributor Author

majgaard commented Jan 19, 2021

@kpunith8 Thanks for spotting that! b573dca should take care of that. Also had to regenerate snapshots because of the updated serializer.

@jdelStrother
Copy link
Contributor

Can anyone suggest what a good cacheKey for NonceProvider is? I'm not familiar with emotion, so I'm not sure whether it's supposed to be a different cacheKey for each component, or each nonce, or each page ..?

@Methuselah96
Copy link
Collaborator

I believe it should be a unique cacheKey per NonceProvider (i.e., a unique cacheKey per Emotion cache since each NonceProvider has its own cache).

@ebonow
Copy link
Collaborator

ebonow commented Jan 30, 2021

I added a note to #3006 to add examples to the documentation for NonceProvider. I struggled finding adequate information on this trying to search through Emotions's documentation.

@jdelStrother
Copy link
Contributor

jdelStrother commented Jan 30, 2021 via email

@Methuselah96
Copy link
Collaborator

@Andarist @mitchellhamilton Can you provide some guidance here? We provide a NonceProvider component that can be used to wrap the Select component in order to add nonce support. What should be the scope of the key being passed into createEmotionCache (i.e., per app, per component, per component instance)?

@Andarist
Copy link

The scope of the key should be, well, per the cache instance and ideally, no 2 caches should use the same key. If you share a single cache through different CacheProviders then this should be OK.

@jdelStrother
Copy link
Contributor

The scope of the key should be, well, per the cache instance

Forgive me, I think I must be missing something important in how emotion works or how it implements caching. Doesn't that mean that emotion ought to be responsible for scoping its own cache keys? What would be the benefit in a client supplying the same cache key to different cache instances?

@Andarist
Copy link

What would be the benefit in a client supplying the same cache key to different cache instances

I'm saying that this should not be done - each new cache should have a new key. Caches integrate with the DOM and it's a shared, global resource - hence the need for keys.

@Methuselah96
Copy link
Collaborator

Methuselah96 commented Jan 31, 2021

I'm saying that this should not be done - each new cache should have a new key.

If this is the case then would it be possible for the Emotion cache to generate its own key (one per instance)?

@Andarist
Copy link

Mainly because of SSR

@jdelStrother
Copy link
Contributor

Say if I have two <Select> components on the page, each wrapped in their own . The two NonceProviders share the same nonce. Can both of them use the same cacheKey? If the two select components have different style props, does that change the answer?

Just using a static cacheKey seems like it works fine, but I'm not really sure what I'm breaking by doing so.

@ebonow
Copy link
Collaborator

ebonow commented Feb 1, 2021

Say if I have two <Select> components on the page, each wrapped in their own . The two NonceProviders share the same nonce. Can both of them use the same cacheKey? If the two select components have different style props, does that change the answer?

Just using a static cacheKey seems like it works fine, but I'm not really sure what I'm breaking by doing so.

My suspicion would be that you may run into issues if you try to apply two different styles to the same component if the using the same cacheKey.

For instance if you want to set the border red in one of your Selects,

const styles = { control: css => ({ ...css, borderColor: 'red' }) }

It could be merging all of the styles together and likely would result in all Selects have a red border. I could be wrong though, and would be interested in seeing the impact or having better familiarity with the impact.

@Andarist
Copy link

Andarist commented Feb 2, 2021

Say if I have two Select components on the page, each wrapped in their own . The two NonceProviders share the same nonce. Can both of them use the same cacheKey?

No - they couldn't use the same cacheKey but you could share the very same cache instance for both, resulting in the same cacheKey being used without any problems. The difference is that you suggest using 2 caches while I propose using a single cache. You just can't create caches per NonceProvider in this scenario - you can still create them lazily there but you'd have to use memoization to actually reuse the same cache for the second NonceProvider.

It could be merging all of the styles together and likely would result in all Selects have a red border. I could be wrong though, and would be interested in seeing the impact or having better familiarity with the impact.

I'm not aware of how React Select works but I would suspect that this is not true - styles should be still scoped per component and Emotion caches don't have much to do with that. They are mainly responsible for injecting a given ruleset once to the CSSOM (and have some global-like options).

@jdelStrother
Copy link
Contributor

Thanks @Andarist, that clears things up.

For the react-select people - I don't think NonceProvider is ideal for our use case, where we have a multi-root react app (boring old static html pages, with multiple react components mounted at different locations on the page). I ended up re-implementing our own version of NonceProvider to share a single cache with a fixed key. A tiny snippet in case it helps anyone else:

import { CacheProvider } from "@emotion/react";
import createCache from "@emotion/cache";
import memoizeOne from "memoize-one";
import cspNonce from "lib/csp_nonce";

const sharedCache = memoizeOne((nonce) =>
  createCache({ nonce, key: "ab-react-select" })
);
function withNonceProvider<Config extends {}>(
  Component: React.ComponentType<Config>
): React.ComponentType<Config> {
  return (props: any) => (
    <CacheProvider value={sharedCache(cspNonce())}>
      <Component {...props} />
    </CacheProvider>
  );
}

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.