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

[docs] Fix iframe demos with emotion #24232

Merged
merged 3 commits into from
Jan 3, 2021
Merged

[docs] Fix iframe demos with emotion #24232

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
53e0676
[docs] Fix iframe demos with emotion
oliviertassinari Jan 3, 2021
797048d
fix rendering
oliviertassinari Jan 3, 2021
d3be514
rerun ci
oliviertassinari Jan 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 17 additions & 3 deletions docs/src/modules/components/DemoSandboxed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import * as React from 'react';
import ReactDOM from 'react-dom';
import PropTypes from 'prop-types';
import { create } from 'jss';
import createCache from '@emotion/cache';
import { CacheProvider } from '@emotion/react';
import { makeStyles, useTheme, jssPreset, StylesProvider } from '@material-ui/core/styles';
import rtl from 'jss-rtl';
import DemoErrorBoundary from 'docs/src/modules/components/DemoErrorBoundary';
Expand All @@ -25,13 +27,25 @@ function FramedDemo(props) {
};
}, [document]);

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

prepend: true,
container: document.body,
}),
[document],
);

const getWindow = React.useCallback(() => document.defaultView, [document]);

return (
<StylesProvider jss={jss} sheetsManager={sheetsManager}>
{React.cloneElement(children, {
window: getWindow,
})}
<CacheProvider value={cache}>
{React.cloneElement(children, {
window: getWindow,
})}
</CacheProvider>
</StylesProvider>
);
}
Expand Down