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

Add insertionPoint option in EmotionCache #2521

Merged
merged 13 commits into from
Nov 3, 2021

Conversation

mnajdova
Copy link
Contributor

@mnajdova mnajdova commented Oct 20, 2021

What:

Fixes #2037 (it is closed, but the feature request haven't been implemented before).

It adds an option to the EmotionCache for specifying an insertion point - dom node after which the style rules will be inserted.

Why:

It is necessary feature for applications that have third-party styles coming from one place, which should be injected first, then the app's styles coming from emotion, for example, coming from a UI library (as @mui/material) that needs to be injected next, and then any other overrides for the default styles.

How:

It's pretty much self explanatory, the style rules are being injected after the specified node. This option wins over prepend, if both are being specified, as I would say is more specific.

Checklist:

  • Documentation
  • Tests
  • Code complete
  • Changeset

@changeset-bot
Copy link

changeset-bot bot commented Oct 20, 2021

🦋 Changeset detected

Latest commit: 61fcc7f

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

This PR includes changesets to release 2 packages
Name Type
@emotion/cache Minor
@emotion/sheet Minor

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 Oct 20, 2021

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 61fcc7f:

Sandbox Source
Emotion Configuration

@codecov
Copy link

codecov bot commented Oct 20, 2021

Codecov Report

Merging #2521 (ef3412f) into main (4be3391) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head ef3412f differs from pull request most recent head 61fcc7f. Consider uploading reports for the commit 61fcc7f to get more accurate results
| Impacted Files | Coverage Δ | |
|---|---|---|
| packages/cache/src/index.js | 97.80% <ø> (ø) | |
| packages/sheet/src/index.js | 100.00% <100.00%> (ø) | |

@@ -37,6 +37,7 @@ export interface Options {
container?: HTMLElement
speedy?: boolean
prepend?: boolean
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
prepend?: boolean
/** @deprecate use `insertionPoint` instead */
prepend?: boolean

a note about this deprecation should both go into the docs and into the changeset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would keep the prepend option, it is much simplified version for that use-case (adding the style rules first in the head). Otherwise you would need to create a node and add it first in the head, just so that you can say, add the style rules after this element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw I am adding the style rules after the node specified in the insertionPoint

Copy link
Member

Choose a reason for hiding this comment

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

I would keep the prepend option, it is much simplified version for that use-case (adding the style rules first in the head). Otherwise you would need to create a node and add it first in the head, just so that you can say, add the style rules after this element.

I was thinking about this in the last few days and I think I would still prefer to deprecate the prepend. This would align more with Emotion's overall philosophy (minimal API surface, only a few options, not using conflicting options etc). While achieving prepend with insertionPoint is a little bit more involved it can still be done. I expect that not that many people need any of those options anyway (both didn't exist in Emotion 10) and this has to be usually only done once in the application.

Removing this option would simplify the code (the cost ain't high here though), but importantly it would simplify the docs, teachability and would allow us to drop conflicting options in the future (insertionPoint used together with prepend doesn't make any sense).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

packages/sheet/src/index.js Outdated Show resolved Hide resolved
@@ -51,6 +51,10 @@ This defines how rules are inserted. If it is true, rules will be inserted with

This defines where rules are inserted into the `container`. By default they are appended but this can be changed by using `prepend: true` option.

#### insertionPoint

This defines specific dom node after which the rules are inserted into the `container`.
Copy link
Member

Choose a reason for hiding this comment

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

It might not always be obvious which element one should choose as their insertionPoint. Even if a correct element gets chosen the relation between this element and it being insertion point might not be obvious.

To make things easier for people and to alleviate the indirect relationship problem we could suggest here using smth like this as the insertion point:

<meta name="emotion-insertion-point" content="" />

This technique (using custom <meta/>) is used by next/head.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with a codesnippet

@Andarist Andarist merged commit 516fe45 into emotion-js:main Nov 3, 2021
// <meta name="emotion-insertion-point" content="">
const emotionInsertionPoint = document.createElement('meta')
emotionInsertionPoint.setAttribute('name', 'emotion-insertion-point')
emotionInsertionPoint.setAttribute('content', '')
Copy link
Member

Choose a reason for hiding this comment

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

FYI: I've checked the spec and it seems that content attribute is required in this case:

If either name, http-equiv, or itemprop is specified, then the content attribute must also be specified. Otherwise, it must be omitted.

Choose a reason for hiding this comment

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

Kindly, does this work with SSR, 'cause document seems to be undefined in my case? What do you suggest I do?
Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

You need to create such a tag (or similar) in your SSR response and then select it on the client's side and pass it as the insertionPoint to createCache

Copy link

@power-f-GOD power-f-GOD Feb 5, 2022

Choose a reason for hiding this comment

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

Hi, @Andarist. Thanks for your response. I saw and did as you suggested and it worked. Thank you.

But then, there's another issue which I'm noticing. I'm using Framer Motion's page transition feature--AnimatePresence--with MUI, and it happens that on route change start, the styles of the unmounting page/component (seem) to get removed from the DOM causing a FOUC of the unmounting page (before it transitions out).

Is there a way to preserve the styles of the pages as they get mounted, so they don't have to get removed and re-added everytime they get mounted/unmounted. I hope you understand what I mean?

Cc: @oliverturner

Copy link
Member

Choose a reason for hiding this comment

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

What styles are removed on the route change? By default Emotion doesn't unmount any styles - except global ones when used through <Global/>. Framer Motion also works in a way that it temporarily preserve components that are unmounted in the tree (they just render exiting children for longer than you are rendering them - they can do that cause they just cache previously received children). So even if some styles would get unmounted during the process they should be unmounted after the component goes completely out of the page so there should be no FOUC.

So while I understand what you are observing there, I don't understand how this can happen. Do you have a repro case (even a complex one, like a full deployed page) that I could take a look at?

Copy link

@power-f-GOD power-f-GOD Feb 6, 2022

Choose a reason for hiding this comment

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

...versus what it currently looks like in prod:

prod-480x.mov

Choose a reason for hiding this comment

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

@Andarist , so you'll notice the FOUC in the production (build) video.

Copy link

@power-f-GOD power-f-GOD Feb 9, 2022

Choose a reason for hiding this comment

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

Hi, @Andarist . Seen these yet?

Copy link
Member

Choose a reason for hiding this comment

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

Could you perhaps record this with https://www.replay.io/ ? I'd like to debug this and can't do that with just a standard recording of the problem.

Choose a reason for hiding this comment

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

Alright, I will. Thanks for your support. 🤲🏼

@Andarist
Copy link
Member

Andarist commented Nov 3, 2021

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.

Ability to insert styles at a specific position in <head>
3 participants