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.

Sign up for GitHub

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

Jump to bottom

[Emotion] Use default cache for consumers who do not pass a cache to EuiProvider #6126

Merged
merged 7 commits into from
Aug 15, 2022
Merged

[Emotion] Use default cache for consumers who do not pass a cache to EuiProvider #6126

merged 7 commits into from
Aug 15, 2022

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Aug 11, 2022 •
edited
Loading

Summary

closes #6113

#5920 did not account for consumers who do not pass a cache tag to <EuiProvider>. This PR fixes the first-child/nth-child console warnings for that scenario, using the "Option 3" method recommended by emotion-js/emotion#1105 (comment) by creating our own default cache.

Checklist

  • Revert [REVERT ME] commit
  • Checked in Chrome, Safari, Edge, and Firefox
  • Added or updated jest and cypress tests
  • A changelog entry exists and is marked appropriately

- [ ] Checked in both light and dark modes
- [ ] Checked in mobile
- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation
- [ ] Checked Code Sandbox works for any docs examples
- [ ] Checked for breaking changes and labeled appropriately
- [ ] Checked for accessibility including keyboard-only and screenreader modes
- [ ] Updated the Figma library counterpart

@cee-chen cee-chen requested a review from thompsongl August 11, 2022 03:03
@cee-chen cee-chen changed the title [Emotion] Use default cache to consumers who do not pass a cache to EuiProvider [Emotion] Use default cache for consumers who do not pass a cache to EuiProvider Aug 11, 2022
@cee-chen
Copy link
Contributor Author

cee-chen commented Aug 11, 2022 •
edited
Loading

Hmm, I was hoping @emotion/css was already an implicit dependency of @emotion/react and that this wouldn't add a bunch of extra deps, but alas 🙈 Not sure if want to consider another approach because of that - perhaps using our own createCache() in source rather than importing from Emotion?

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6126/

@thompsongl
Copy link
Contributor

Not sure if want to consider another approach because of that - perhaps using our own createCache() in source rather than importing from Emotion?

Yeah I think I'd rather replicate the default cache without pulling in @emotion/css

const defaultCache = createCache({ key: 'css' });
defaultCache.compat = true;

@cee-chen
Copy link
Contributor Author

cee-chen commented Aug 11, 2022 •
edited
Loading

FYI it looks like the main difference between our own createCache() default and @emotion/css's cache export is a speedy fn:

Not sure how much that matters and I don't think it's a big deal, just wanted to mention it for future context!

@cee-chen cee-chen force-pushed the default-emotion-cache branch from cf9a7b3 to a04a508 Compare August 11, 2022 16:32
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6126/

thompsongl
thompsongl reviewed Aug 11, 2022
View reviewed changes
src/components/provider/cache/cache_provider.tsx
@@ -7,20 +7,19 @@
*/

import React, { PropsWithChildren } from 'react';
import { EmotionCache } from '@emotion/cache';
import createCache, { EmotionCache } from '@emotion/cache';
Copy link
Contributor

Choose a reason for hiding this comment

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

Just realized that we have @emotion/cache as a devDep. We'll need to move it to dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since @emotion/react is listed as a peerDependency, does it also make sense to make @emotion/cache a peerDependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

6320248

@cee-chen cee-chen force-pushed the default-emotion-cache branch from 57cd674 to 6320248 Compare August 12, 2022 00:24
@cee-chen
Copy link
Contributor Author

Also going to go ahead and revert the revert example - assuming you've had a chance to QA staging already

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6126/

@cee-chen cee-chen requested a review from thompsongl August 12, 2022 16:05
thompsongl
thompsongl reviewed Aug 12, 2022
View reviewed changes
upcoming_changelogs/6126.md
@@ -0,0 +1,3 @@
**Bug fixes**
Copy link
Contributor

Choose a reason for hiding this comment

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

The change in peerDependencies technically makes this a breaking change 😿

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops, right! 🤦

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6126/

@cee-chen cee-chen requested a review from thompsongl August 15, 2022 15:47
@cee-chen cee-chen merged commit f0f8360 into elastic:main Aug 15, 2022
@cee-chen cee-chen deleted the default-emotion-cache branch August 15, 2022 16:17
@1Copenut 1Copenut mentioned this pull request Sep 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thompsongl thompsongl thompsongl approved these changes

Assignees
No one assigned
Labels
breaking change emotion
Projects
None yet
Milestone
No milestone
3 participants
@cee-chen @kibanamachine @thompsongl