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

[docs] Fix iframe demos with emotion #24232

Merged
merged 3 commits into from Jan 3, 2021

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jan 3, 2021

The problem was raised in #24097 (comment) and can be seen on:

Capture d’écran 2021-01-03 à 10 57 51

https://next--material-ui.netlify.app/components/drawers/#persistent-drawer

It's a blocker to merge #24227 as the container only makes sense for a top-level layout, all the demos use an iframe.

Preview: https://deploy-preview-24232--material-ui.netlify.app/components/drawers/#persistent-drawer

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Jan 3, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Jan 3, 2021

Details of bundle changes

Generated by 🚫 dangerJS against d3be514

const cache = React.useMemo(
() =>
createCache({
key: 'iframe-demo',
Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that we can share the same cache between two iframe demos. The documentation only mentions that unique keys are required when using the same window/frame https://emotion.sh/docs/@emotion/cache#key.

@Andarist is it accurate?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe that you can share the same cache between iframes because each one has to get their separate styles in their separate documents. Cache only checks the inserted keys and assumes that inserted keys are in sync with the inserted style elements - but if you share cache between "realms" then styles will really be only inserted into a single document.

Copy link
Contributor

Choose a reason for hiding this comment

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

Separate note - useMemo doesn't give any semantic guarantees about caching (it's just like a perf hint). This probably doesn't matter for you here but to avoid useMemo refiring (even if document doesn't change!) you should put cache into a ref or into a state.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cache only checks the inserted keys and assumes that inserted keys are in sync with the inserted style elements

In the current configuration, each iframe has a different emotion cache instance in the React context with the same key. Is there a global side effect between different cache instances? (the document global side effect is not happening as we use different document, one per iframe). So far, the demos are rendered correctly.

useMemo doesn't give any semantic guarantees about caching

Would the style be broken if the emotion cache changes between two renders? I ask because I'm happy with a perf hint only.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a global side effect between different cache instances?

There is:

https://github.com/emotion-js/emotion/blob/6e157aa198ac32d326d1aa1a921f459d880a8428/packages/cache/src/index.js#L84-L99

But it operates on the main script's document so it should simply not find any iframe styles in it and this should act like a noop - like you have mentioned.

Would the style be broken if the emotion cache changes between two renders? I ask because I'm happy with a perf hint only.

No - styles should not be affected. You would only end up with multiple copies of the same styles being injected and some perf hit caused by somewhat full subtree rerended (all context consumers would have to rerender)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok great, awesome, thanks for the details

@oliviertassinari oliviertassinari merged commit 3ea2f14 into mui:next Jan 3, 2021
@oliviertassinari oliviertassinari deleted the docs-fix-portal-demos branch January 3, 2021 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants