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

Add insertionPoint option in EmotionCache #2521

Merged
merged 13 commits into from Nov 3, 2021
Merged

Add insertionPoint option in EmotionCache #2521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
84cf0a4
Add insertionPoint option in EmotionCache
mnajdova Oct 20, 2021
a8a38cc
yarn changeset
mnajdova Oct 20, 2021
5e801e0
yarn lint
mnajdova Oct 20, 2021
221c298
Add tests as per view
mnajdova Oct 21, 2021
60bf5bd
Updated changeset, improved tests
mnajdova Oct 21, 2021
b13a72c
Update packages/sheet/src/index.js
mnajdova Oct 25, 2021
3fc9c88
Update packages/cache/__tests__/index.js
mnajdova Oct 27, 2021
c8fa22d
Improve tests
mnajdova Oct 27, 2021
1c94b62
Use @testing-library
mnajdova Oct 27, 2021
74ab823
Address comments from review
mnajdova Oct 27, 2021
218ea87
Fix some flow issues
mnajdova Oct 27, 2021
ef3412f
fix selectors
mnajdova Oct 27, 2021
61fcc7f
small tweaks
Andarist Nov 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
31 changes: 31 additions & 0 deletions .changeset/sixty-balloons-build.md
View file
Open in desktop
@@ -0,0 +1,31 @@
---
'@emotion/cache': minor
'@emotion/sheet': minor
---

Add insertionPoint option to the EmotionCache, to insert rules after the specified element.
mnajdova marked this conversation as resolved.
Show resolved Hide resolved

```jsx
const head = document.querySelector('head')

// <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.

Copy link

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

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

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

Copy link

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

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.

Copy link

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. 🤲🏼


head.appendChild(emotionInsertionPoint)

// the emotion sheets should be inserted right after the meta tag
const cache = createCache({
key: 'my-app',
insertionPoint: emotionInsertionPoint
})

function App() {
return (
<CacheProvider value={cache}>
<Main />
</CacheProvider>
)
}
```
24 changes: 24 additions & 0 deletions packages/cache/__tests__/__snapshots__/index.js.snap
View file
Open in desktop
@@ -1,3 +1,27 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`should accept insertionPoint option 1`] = `
<head>


<style
id="first"
/>
<style
data-emotion="test-insertion-point"
data-s=""
>

.test-insertion-point-83n355{display:-webkit-box;display:-webkit-flex;display:-ms-flexbox;display:flex;color:blue;}
</style>


<style
id="last"
/>


</head>
`;

exports[`throws correct error with invalid key 1`] = `"Emotion key must only contain lower case alphabetical characters and - but \\".\\" was passed"`;
28 changes: 28 additions & 0 deletions packages/cache/__tests__/index.js
View file
Open in desktop
@@ -1,8 +1,36 @@
// @flow
/** @jsx jsx */
import 'test-utils/next-env'
import { safeQuerySelector } from 'test-utils'
import createCache from '@emotion/cache'
import { jsx, CacheProvider } from '@emotion/react'
import { render } from '@testing-library/react'

test('throws correct error with invalid key', () => {
expect(() => {
createCache({ key: '.' })
}).toThrowErrorMatchingSnapshot()
})

it('should accept insertionPoint option', () => {
const head = safeQuerySelector('head')

head.innerHTML = `
<style id="first"></style>
<style id="last"></style>
`

// the sheet should be inserted between the first and last style nodes
const cache = createCache({
key: 'test-insertion-point',
insertionPoint: safeQuerySelector('#first')
})

render(
<CacheProvider value={cache}>
<div css={{ display: 'flex', color: 'blue' }} />
</CacheProvider>
)

expect(document.head).toMatchSnapshot()
})
6 changes: 4 additions & 2 deletions packages/cache/src/index.js
View file
Open in desktop
Expand Up @@ -28,7 +28,8 @@ export type Options = {
key: string,
container?: HTMLElement,
speedy?: boolean,
prepend?: boolean
prepend?: boolean,
insertionPoint?: HTMLElement
mnajdova marked this conversation as resolved.
Show resolved Hide resolved
}

let getServerStylisCache = isBrowser
Expand Down Expand Up @@ -252,7 +253,8 @@ let createCache = (options: Options): EmotionCache => {
container: ((container: any): HTMLElement),
nonce: options.nonce,
speedy: options.speedy,
prepend: options.prepend
prepend: options.prepend,
insertionPoint: options.insertionPoint
}),
nonce: options.nonce,
inserted,
Expand Down
2 changes: 2 additions & 0 deletions packages/cache/types/index.d.ts
View file
Open in desktop
Expand Up @@ -36,7 +36,9 @@ export interface Options {
key: string
container?: HTMLElement
speedy?: boolean
/** @deprecate use `insertionPoint` instead */
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

insertionPoint?: HTMLElement
}

export default function createCache(options: Options): EmotionCache
31 changes: 31 additions & 0 deletions packages/sheet/README.md
View file
Open in desktop
Expand Up @@ -49,8 +49,39 @@ This defines how rules are inserted. If it is true, rules will be inserted with

#### prepend

**Deprecated:** Please use `insertionPoint` option instead.

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`. You can use a `meta` tag to specify the specific location:

```jsx
const head = document.querySelector('head')

// <meta name="emotion-insertion-point" content="">
const emotionInsertionPoint = document.createElement('meta')
emotionInsertionPoint.setAttribute('name', 'emotion-insertion-point')
emotionInsertionPoint.setAttribute('content', '')

head.appendChild(emotionInsertionPoint)

// the emotion sheets should be inserted right after the meta tag
const cache = createCache({
key: 'my-app',
insertionPoint: emotionInsertionPoint
})

function App() {
return (
<CacheProvider value={cache}>
<Main />
</CacheProvider>
)
}
```

### Methods

#### insert
Expand Down
59 changes: 59 additions & 0 deletions packages/sheet/__tests__/__snapshots__/index.js.snap
View file
Open in desktop
@@ -1,5 +1,39 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`StyleSheet should accept insertionPoint option 1`] = `
<html>
<head>


<style
id="first"
/>
<style
data-emotion=""
data-s=""
>

html { color: hotpink; }
</style>
<style
data-emotion=""
data-s=""
>

* { box-sizing: border-box; }
</style>


<style
id="last"
/>


</head>
<body />
</html>
`;

exports[`StyleSheet should accept prepend option 1`] = `
<html>
<head>
Expand Down Expand Up @@ -212,3 +246,28 @@ exports[`StyleSheet should use the container option instead of document.head to
</body>
</html>
`;

exports[`StyleSheet should work if insertionPoint is last element 1`] = `
<html>
<head>
<style
id="last"
/>
<style
data-emotion=""
data-s=""
>

html { color: hotpink; }
</style>
<style
data-emotion=""
data-s=""
>

* { box-sizing: border-box; }
</style>
</head>
<body />
</html>
`;
47 changes: 43 additions & 4 deletions packages/sheet/__tests__/index.js
View file
Open in desktop
Expand Up @@ -17,6 +17,11 @@ afterEach(() => {
jest.clearAllMocks()
})

beforeEach(() => {
safeQuerySelector('head').innerHTML = ''
safeQuerySelector('body').innerHTML = ''
})

describe('StyleSheet', () => {
it('should be speedy by default in production', () => {
process.env.NODE_ENV = 'production'
Expand Down Expand Up @@ -98,8 +103,6 @@ describe('StyleSheet', () => {
expect(sheet.tags).toHaveLength(1)
expect(sheet.tags[0].parentNode).toBe(container)
sheet.flush()
// $FlowFixMe
document.body.removeChild(container)
})

it('should accept prepend option', () => {
Expand All @@ -114,7 +117,44 @@ describe('StyleSheet', () => {
expect(document.documentElement).toMatchSnapshot()

sheet.flush()
head.removeChild(otherStyle)
})

it('should accept insertionPoint option', () => {
const head = safeQuerySelector('head')

head.innerHTML = `
<style id="first"></style>
<style id="last"></style>
`

// the sheet should be inserted between the first and last style nodes
const sheet = new StyleSheet({
...defaultOptions,
insertionPoint: safeQuerySelector('#first')
})
sheet.insert(rule)
sheet.insert(rule2)
expect(document.documentElement).toMatchSnapshot()

sheet.flush()
})

it('should work if insertionPoint is last element', () => {
const head = safeQuerySelector('head')
const lastStyle = document.createElement('style')
lastStyle.setAttribute('id', 'last')
head.appendChild(lastStyle)

// the sheet should be inserted after the first node
const sheet = new StyleSheet({
...defaultOptions,
insertionPoint: lastStyle
})
sheet.insert(rule)
sheet.insert(rule2)
expect(document.documentElement).toMatchSnapshot()

sheet.flush()
})

it('should be able to hydrate styles', () => {
Expand Down Expand Up @@ -179,7 +219,6 @@ describe('StyleSheet', () => {
expect(document.documentElement).toMatchSnapshot()

sheet.flush()
head.removeChild(otherStyle)
})

it('should not crash when flushing when styles are already detached', () => {
Expand Down
13 changes: 11 additions & 2 deletions packages/sheet/src/index.js
View file
Open in desktop
Expand Up @@ -44,7 +44,8 @@ export type Options = {
key: string,
container: HTMLElement,
speedy?: boolean,
prepend?: boolean
prepend?: boolean,
insertionPoint?: HTMLElement
}

function createStyleElement(options: {
Expand All @@ -70,6 +71,7 @@ export class StyleSheet {
nonce: string | void
prepend: boolean | void
before: Element | null
insertionPoint: HTMLElement | void
constructor(options: Options) {
this.isSpeedy =
options.speedy === undefined
Expand All @@ -82,13 +84,20 @@ export class StyleSheet {
this.key = options.key
this.container = options.container
this.prepend = options.prepend
this.insertionPoint = options.insertionPoint
this.before = null
}

_insertTag = (tag: HTMLStyleElement) => {
let before
if (this.tags.length === 0) {
before = this.prepend ? this.container.firstChild : this.before
if (this.insertionPoint) {
before = this.insertionPoint.nextSibling
} else if (this.prepend) {
before = this.container.firstChild
} else {
before = this.before
}
} else {
before = this.tags[this.tags.length - 1].nextSibling
}
Expand Down
2 changes: 2 additions & 0 deletions packages/sheet/types/index.d.ts
View file
Open in desktop
Expand Up @@ -6,7 +6,9 @@ export interface Options {
key: string
container: HTMLElement
speedy?: boolean
/** @deprecate use `insertionPoint` instead */
prepend?: boolean
insertionPoint?: HTMLElement
}

export class StyleSheet {
Expand Down