Stylis v4

[Andarist]

This is not ready by any means - just an initial attempt at using new Stylis.

It looks really promising, when preparing this I've found some minor problems with Stylis v4, for most of them I've already opened PRs against Stylis. The biggest unsolved problem right now is prefixing by adding a completely new rule (adding webkit keyframes) in combination with rulesheet plugin. Truth to be told - I'm not even sure if we really need this, but from Stylis perspective it should be possible to do this nevertheless.

cc @mitchellhamilton

[changeset-bot]

🦋 Changeset is good to go

Latest commit: 2ec872c

We got this.

This PR includes changesets to release 13 packages

Name	Type
@emotion/babel-plugin	Major
@emotion/sheet	Major
@emotion/cache	Major
@emotion/css	Major
@emotion/react	Major
@emotion/styled	Major
@emotion/utils	Major
@emotion/server	Major
@emotion/jest	Major
@emotion/primitives-core	Major
@emotion/serialize	Patch
@emotion/native	Major
@emotion/primitives	Major

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]

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 2ec872c:

Sandbox	Source
Emotion	Configuration

[codecov]

Codecov Report

Merging #1817 into next will increase coverage by 0.02%.
The diff coverage is 99.21%.

Impacted Files	Coverage Δ
packages/utils/src/index.js	100.00% <ø> (ø)
packages/sheet/src/index.js	97.50% <75.00%> (-2.50%)	⬇️
packages/babel-plugin/src/utils/minify.js	100.00% <100.00%> (ø)
...ugin/src/utils/transform-expression-with-styles.js	95.74% <100.00%> (-0.09%)	⬇️
packages/cache/src/index.js	97.18% <100.00%> (-0.82%)	⬇️
packages/cache/src/stylis-plugins.js	100.00% <100.00%> (ø)
packages/css/test/instance/emotion-instance.js	100.00% <100.00%> (ø)
packages/jest/src/serializer.js	97.67% <0.00%> (-2.33%)	⬇️

[emmatown]

This is looking great!! 🎉 🎉

[emmatown]

@import has to be the first rule(or at least before non @import rules) when using insertRule, could you check if this not being first here is fine?

[Andarist]

It probably is not - I've changed tests to always put @import first: 2d95e2c

Stylis v3 was special-casing this and it actually has moved import rules to the first position when compiling, v4 doesn't do that anymore. It seems to me that it's a valid approach to just require from the user to put those first "manually". Special-casing this would require from us writing a special stylis plugin for the server path - IMHO it's not worth it.

If we decide this being a requirement on the consumer side then this special case could be dropped I think:

emotion/packages/sheet/src/index.js

Line 118 in ff9746b

let isImportRule =

@import rules should not occur in the scoped styles and for global ones they should just be put first. Keep also in mind that @charset should have even higher priority than @import and we do not special case it anyway - it's way less common though.

[emmatown]

Keep also in mind that @charset should have even higher priority than @import and we do not special case it anyway - it's way less common though.

Yeah, I'm not super worried about @charset given I've never seen anyone use it.

Okay - I think I'm fine with requiring @import be first and only be in Global. We definitely need a better error than what we would have if we just removed the isImportRule stuff though.

[Andarist]

Ok, I've pushed out changes around this - together with new dev-only warnings for incorrect inputs.

[Andarist]

Just realized a problem with the @import thing. It will mean that this will be broken:

Good catch, I totally have missed this breaking the vanilla emotion.

I like the idea of making each injectGlobal having its own style tag (it would nicely match how <Global/> behaves) but when I think about it then it seems to complicate the APIs around flushing and .sheet access (although I'm not completely sure why .sheet is available directly). As to flushing - we could maintain a list of sheets inside @emotion/css to flush them all? I have also thought about how this would play with rehydration etc but at the first glance this doesn't seem to be problematic. From what I understand calling hydrate is required anyway and it inserts ids into a cache, not sheets, so everything is fine.

[emmatown]

calling hydrate is required anyway and it inserts ids into a cache, not sheets, so everything is fine.

This isn't true, the same auto-hydration that happens for @emotion/react happens here.

---

So I just ran this code

const { injectGlobal } = require("emotion");
const { renderStylesToString } = require("emotion-server");

injectGlobal`.thing {display:flex;}`;

injectGlobal`@import 'custom.css';`;

console.log(renderStylesToString(""));

It outputs

<style data-emotion-css="njnz1q mndbu7">.thing{display:-webkit-box;display:-webkit-flex;display:-ms-flexbox;display:flex;}@import 'custom.css';</style>

So I'm kind of just inclined to say "@imports must only happen in the first injectGlobal call in your app" and then all of our problems are solved right? As long as we error with a nice message when this happens, I'm okay with it.

[Andarist]

This isn't true, the same auto-hydration that happens for @emotion/react happens here.

You are right, it seems like hydrate is not needed at all then?

So I'm kind of just inclined to say "@imports must only happen in the first injectGlobal call in your app" and then all of our problems are solved right?

If we assume that @import cant be lazily inserted during a page's lifetime - then yeah, there is no problem.

As long as we error with a nice message when this happens, I'm okay with it.

Okay - gonna improve this error a little bit tomorrow:

emotion/packages/sheet/src/index.js

Line 127 in 5e39d81

'`@import` rules must be inserted before other rules.'

[emmatown]

You are right, it seems like hydrate is not needed at all then?

#1876 (comment)

[Andarist]

@mitchellhamilton I've improved the errors logged on incorrect @import usage in the latest commit: a1d7173 . Please take a look if this looks good to you. We also have other, somewhat lighter/more-specific warnings concerned with a single stylis call which you can check out in action here:

emotion/packages/react/__tests__/warnings.js

Lines 256 to 311 in a1d7173

	test('@import nested in scoped `css`', () => {
	renderer.create(
	<div
	css={css`
	@import url('https://some-url');
	h1 {
	color: hotpink;
	}
	`}
	/>
	)
	expect((console.error: any).mock.calls).toMatchInlineSnapshot(`
	Array [
	Array [
	"\`@import\` rules can't be nested inside other rules. Please move it to the top and put it before regular rules. Keep in mind that they can only be used within global styles.",
	],
	]
	`)
	})
	test('@import prepended with other rules', () => {
	renderer.create(
	<Global
	styles={css`
	h1 {
	color: hotpink;
	}
	@import url('https://some-url');
	`}
	/>
	)
	expect((console.error: any).mock.calls).toMatchInlineSnapshot(`
	Array [
	Array [
	"\`@import\` rules can't be preceded by regular rules. Please put it before them.",
	],
	]
	`)
	})
	test('@import prepended by other @import', () => {
	renderer.create(
	<Global
	styles={css`
	@import url('https://some-url');
	@import url('https://some-url2');
	`}
	/>
	)
	expect((console.error: any).mock.calls).toMatchInlineSnapshot(`Array []`)
	})

, this new commit improves a situation when @import gets inserted incorrectly between stylis calls. Keep also in mind that this mostly affects vanilla Emotion users as @emotion/react scopes <Global/> styles to separate <style/> elements.

I believe this was the last remaining item here so if you approve this then we should finally be ready to merge this 🎉

[emmatown]

Just some little nits. I assume you're gonna update docs in another PR or something?

[emmatown]

Not really a blocker but why not make this an optional property on the class?

[Andarist]

Just a minor difference - an optional property would be visible as part of the public type and we don't need it to be there.

[emmatown]

You should tell users where to get the default prefixer from here.

[Andarist]

Added such mention just now.

[Andarist]

I assume you're gonna update docs in another PR or something?

Do you have anything in mind that needs to be documented besides vendor-prefixing stuff covered by those 2nd and 3rd points of this changeset:

emotion/.changeset/warm-ties-drop.md

Lines 12 to 13 in 703b1dd

- vendor-prefixing was previously customizable using `prefix` option. This was always limited to turning off all of some of the prefixes as all available prefixes were on by default. The `prefix` option is gone and to customize which prefixes are applied you need to fork (copy-paste) the prefixer plugin and adjust it to your needs. While this being somewhat more problematic to setup at first we believe that the vast majority of users were not customizing this anyway. By not including the possibility to customize this through an extra option the final solution is more performant because there is no extra overhead of checking if a particular property should be prefixed or not.

- Prefixer is now just a plugin which happens to be put in default `stylisPlugins`. If you plan to use custom `stylisPlugins` and you want to have your styles prefixed automatically you must include prefixer in your custom `stylisPlugins`. You can import `prefixer` from the `stylis` module to do that.

[emmatown]

Stuff like https://github.com/emotion-js/emotion/blob/master/docs/cache-provider.mdx, https://github.com/emotion-js/emotion/tree/master/packages/cache#options and potentially anything else that exists.

[zenininja]

Stylis upgrade seems to have broken the React Native compatibility :(. Please check here #1986