chore: custom sheet #2677

Jack-Works · 2022-03-09T10:29:32Z

What: change new StyleSheet to new cache.sheet.constructor

Why: close #2675

How:

Checklist:

Documentation
Tests
Code complete
Changeset

changeset-bot · 2022-03-09T10:29:35Z

🦋 Changeset detected

Latest commit: ea74c7a

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

This PR includes changesets to release 1 package

Name	Type
@emotion/react	Patch

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

Jack-Works · 2022-03-09T10:30:50Z

hi! I cannot run jest -u to update the snapshot cause it complains

 FAIL  packages/react/__tests__/custom-cache.js
  ● Test suite failed to run

    SyntaxError: C:\Users\Jack\Workspace\emotion\packages\react\__tests__\custom-cache.js: Support for the experimental 
syntax 'jsx' isn't currently enabled (27:7):

      25 |   expect(
      26 |     render(
    > 27 |       <CacheProvider value={cache}>
         |       ^
      28 |         <div css={{ display: 'flex', color: 'blue' }} />
      29 |       </CacheProvider>
      30 |     )

    Add @babel/preset-react (https://git.io/JfeDR) to the 'presets' section of your Babel config to enable transformation.
    If you want to leave it as-is, add @babel/plugin-syntax-jsx (https://git.io/vb4yA) to the 'plugins' section to enable parsing.

      at Parser._raise (node_modules/jest-cli/node_modules/@babel/parser/src/parser/error.js:147:45)

codesandbox-ci · 2022-03-09T10:31:38Z

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 ea74c7a:

Sandbox	Source
Emotion	Configuration

packages/react/src/global.js

Andarist · 2022-03-09T10:36:14Z

packages/react/__tests__/custom-cache.js

+      if (Reflect.get(options.container, 'sheet')) {
+        return new GlobalSheet(options)
+      } else {
+        Reflect.set(options.container, 'sheet', this)
+      }


I wonder - any particular reason why Reflect is used here?

To bypass the flow type check. sheet is not a property on the HTMLElement

Andarist · 2022-03-09T10:37:11Z

hi! I cannot run jest -u to update the snapshot cause it complains

That is somewhat weird but don't worry - I will update them before merging

codecov · 2022-03-09T11:59:10Z

Codecov Report

Merging #2677 (4b790e4) into main (4266aa0) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 4b790e4 differs from pull request most recent head ea74c7a. Consider uploading reports for the commit ea74c7a to get more accurate results

Impacted Files	Coverage Δ
packages/react/src/global.js	`98.24% <100.00%> (ø)`

Andarist · 2022-03-09T22:54:53Z

I've wanted to push out changes to this branch - but it seems like I don't have sufficient permissions to push to your fork.

You can check how to allow this for future PRs here - https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork . At the moment, you can try applying this patch:

diff --git a/packages/react/__tests__/custom-cache.js b/packages/react/__tests__/custom-cache.js
index cab71fc3..86f6d89f 100644
--- a/packages/react/__tests__/custom-cache.js
+++ b/packages/react/__tests__/custom-cache.js
@@ -1,10 +1,11 @@
 // @flow
 /** @jsx jsx */
-import 'test-utils/next-env'
 import createCache from '@emotion/cache'
-import { jsx, CacheProvider, Global } from '@emotion/react'
+import { CacheProvider, Global, jsx } from '@emotion/react'
 import { StyleSheet } from '@emotion/sheet'
 import renderer from 'react-test-renderer'
+import { safeQuerySelector } from 'test-utils'
+import 'test-utils/next-env'
 
 function stylisPlugin(element) {
   if (element.type === 'decl' && element.value.startsWith('color:')) {
@@ -16,6 +17,11 @@ function render(ele) {
   return renderer.create(ele).toJSON()
 }
 
+beforeEach(() => {
+  safeQuerySelector('head').innerHTML = ''
+  safeQuerySelector('body').innerHTML = ''
+})
+
 test('with custom plugins', () => {
   let cache = createCache({
     key: 'custom-plugins',
@@ -40,40 +46,42 @@ test('with custom plugins', () => {
   `)
 })
 
-test('with custom sheet', () => {
+test('Global should "inherit" sheet class from the cache', () => {
   // https://github.com/emotion-js/emotion/issues/2675
   let cache = createCache({
     key: 'test',
     speedy: false
   })
-
-  class GlobalSheet extends StyleSheet {
-    insert(rule) {
-      return super.insert('/** global */' + rule)
-    }
-  }
   class MySheet extends StyleSheet {
-    constructor(options) {
-      super(options)
-      if (Reflect.get(options.container, 'sheet')) {
-        return new GlobalSheet(options)
-      } else {
-        Reflect.set(options.container, 'sheet', this)
-      }
-    }
-
     insert(rule) {
       super.insert(`/** ${this.key} */${rule}`)
     }
   }
   cache.sheet = new MySheet({ key: 'test', container: document.head })
 
-  expect(
-    render(
-      <CacheProvider value={cache}>
-        <div css={{ display: 'flex', color: 'blue' }} />
-        <Global styles={{ body: { width: '0' } }} />
-      </CacheProvider>
-    )
-  ).toMatchInlineSnapshot()
+  render(
+    <CacheProvider value={cache}>
+      <div css={{ color: 'hotpink' }} />
+      <Global styles={{ body: { width: '0' } }} />
+    </CacheProvider>
+  )
+
+  expect(safeQuerySelector('head')).toMatchInlineSnapshot(`
+    <head>
+      <style
+        data-emotion="test-global"
+        data-s=""
+      >
+        
+        /** test-global */body{width:0;}
+      </style>
+      <style
+        data-emotion="test"
+        data-s=""
+      >
+        
+        /** test */.test-1lrxbo5{color:hotpink;}
+      </style>
+    </head>
+  `)
 })

Andarist · 2022-03-10T11:13:24Z

hi! I cannot run jest -u to update the snapshot cause it complains

That was weird and I've also experienced that. The problem is when you are using .toMatchInlineSnapshot() in this file - Jest tries to automatically parse the file and insert the generated snapshot as the argument to this call and it fails in the process. I didn't ever have this problem in this repo and I'm not sure if we have changed something in our configs recently or what. I'm pretty sure that I was using .toMatchInlineSnapshot() pretty extensively here in the past.

I didn't solve this problem, I just have provided a dummy value as a snapshot manually and then constructed the correct value manually based on the CLI report. I'll have to look into this later but that's a completely separate issue from this PR.

Andarist · 2022-03-10T11:18:30Z

packages/react/__tests__/custom-cache.js

+    key: 'test',
+    speedy: false
+  })
+  class MySheet extends StyleSheet {


I've noticed that you were constructing different sheet classes in the constructor of your custom implementation but perhaps that's not really needed. You can easily differentiate hashed/main and global sheets by just checking the options.key.endsWith('-global'). Based on that you can just assign some private property and branch the logic in the insert method. This way you only have to write a single StyleSheet class which might simplify your implementation.

oh, I need to get the un-global version of the sheet so that's the easiest way, any way, thanks!

Jack-Works · 2022-03-10T11:36:28Z

That was weird and I've also experienced that.

Maybe because I'm using Windows? 🤔

Andarist · 2022-03-10T12:22:17Z

I have meant that the same thing has happened to me - so it was definitely not specific to your machine etc.

Jack-Works added 2 commits March 9, 2022 18:25

chore: add custom sheet

04b265c

chore: add changeset

e638c6d

Andarist reviewed Mar 9, 2022

View reviewed changes

packages/react/src/global.js Show resolved Hide resolved

Andarist reviewed Mar 9, 2022

View reviewed changes

chore: remove unused import

8f6c32f

Jack-Works and others added 4 commits March 10, 2022 09:54

chore: apply patch

c3a2df3

fix inline snapshots

1bb5167

Fix Flow error

4b790e4

tweak changeset

ea74c7a

Andarist merged commit ff3cb16 into emotion-js:main Mar 10, 2022

Andarist reviewed Mar 10, 2022

View reviewed changes

github-actions bot mentioned this pull request Mar 10, 2022

Version Packages #2679

Merged

Jack-Works deleted the custom-sheet branch March 10, 2022 11:36

Andarist mentioned this pull request Aug 2, 2022

Upgrade to Yarn 3 (attempt 3) #2841

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: custom sheet #2677

chore: custom sheet #2677

Jack-Works commented Mar 9, 2022 •

edited

Loading

changeset-bot bot commented Mar 9, 2022 •

edited

Loading

Jack-Works commented Mar 9, 2022

codesandbox-ci bot commented Mar 9, 2022 •

edited

Loading

Andarist Mar 9, 2022

Jack-Works Mar 9, 2022

Andarist commented Mar 9, 2022

codecov bot commented Mar 9, 2022 •

edited

Loading

Andarist commented Mar 9, 2022

Andarist commented Mar 10, 2022

Andarist Mar 10, 2022

Jack-Works Mar 10, 2022

Jack-Works commented Mar 10, 2022

Andarist commented Mar 10, 2022

chore: custom sheet #2677

chore: custom sheet #2677

Conversation

Jack-Works commented Mar 9, 2022 • edited Loading

changeset-bot bot commented Mar 9, 2022 • edited Loading

🦋 Changeset detected

Jack-Works commented Mar 9, 2022

codesandbox-ci bot commented Mar 9, 2022 • edited Loading

Andarist Mar 9, 2022

Choose a reason for hiding this comment

Jack-Works Mar 9, 2022

Choose a reason for hiding this comment

Andarist commented Mar 9, 2022

codecov bot commented Mar 9, 2022 • edited Loading

Codecov Report

Andarist commented Mar 9, 2022

Andarist commented Mar 10, 2022

Andarist Mar 10, 2022

Choose a reason for hiding this comment

Jack-Works Mar 10, 2022

Choose a reason for hiding this comment

Jack-Works commented Mar 10, 2022

Andarist commented Mar 10, 2022

Jack-Works commented Mar 9, 2022 •

edited

Loading

changeset-bot bot commented Mar 9, 2022 •

edited

Loading

codesandbox-ci bot commented Mar 9, 2022 •

edited

Loading

codecov bot commented Mar 9, 2022 •

edited

Loading