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 6 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
29 changes: 29 additions & 0 deletions .changeset/sixty-balloons-build.md
View file
Open in desktop
@@ -0,0 +1,29 @@
---
'@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

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

const firstStyle = document.createElement('style')
const lastStyle = document.createElement('style')
head.appendChild(firstStyle)
head.appendChild(lastStyle)

// the emotion sheets should be inserted between the first and last style nodes
const cache = createCache({
key: 'my-app',
insertionPoint: firstStyle
})

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

exports[`should accept insertionPoint option 1`] = `
<html>
<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>
<body />
</html>
`;

exports[`throws correct error with invalid key 1`] = `"Emotion key must only contain lower case alphabetical characters and - but \\".\\" was passed"`;
34 changes: 34 additions & 0 deletions packages/cache/__tests__/index.js
View file
Open in desktop
@@ -1,8 +1,42 @@
// @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 renderer from 'react-test-renderer'
mnajdova marked this conversation as resolved.
Show resolved Hide resolved

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

function render(ele) {
return renderer.create(ele).toJSON()
}

it('should accept insertionPoint option', () => {
const head = safeQuerySelector('head')
const firstStyle = document.createElement('style')
firstStyle.setAttribute('id', 'first')
head.appendChild(firstStyle)

const lastStyle = document.createElement('style')
lastStyle.setAttribute('id', 'last')
head.appendChild(lastStyle)
mnajdova marked this conversation as resolved.
Show resolved Hide resolved

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

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

expect(document.documentElement).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
1 change: 1 addition & 0 deletions packages/cache/types/index.d.ts
View file
Open in desktop
Expand Up @@ -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

insertionPoint?: HTMLElement
}

export default function createCache(options: Options): EmotionCache
4 changes: 4 additions & 0 deletions packages/sheet/README.md
View file
Open in desktop
Expand Up @@ -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


### Methods

#### insert
Expand Down
53 changes: 53 additions & 0 deletions packages/sheet/__tests__/__snapshots__/index.js.snap
View file
Open in desktop
@@ -1,5 +1,33 @@
// 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 +240,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>
`;
43 changes: 43 additions & 0 deletions packages/sheet/__tests__/index.js
View file
Open in desktop
Expand Up @@ -117,6 +117,49 @@ describe('StyleSheet', () => {
head.removeChild(otherStyle)
})

it('should accept insertionPoint option', () => {
const head = safeQuerySelector('head')
const firstStyle = document.createElement('style')
firstStyle.setAttribute('id', 'first')
head.appendChild(firstStyle)

const lastStyle = document.createElement('style')
lastStyle.setAttribute('id', 'last')
head.appendChild(lastStyle)

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

sheet.flush()
head.removeChild(firstStyle)
head.removeChild(lastStyle)
mnajdova marked this conversation as resolved.
Show resolved Hide resolved
})

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()
head.removeChild(lastStyle)
})

it('should be able to hydrate styles', () => {
const fooStyle = document.createElement('style')
fooStyle.textContent = '.foo { color: hotpink; }'
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