Fix ignoreFlag not works bug #2751

kyoncy · 2022-05-17T13:20:53Z

What:
Fix #2478

When using the ignoreFlag for nth-child being potentially unsafe with server rendering, the first instance of the flag works while the second does not (workaround provided).

Why:
To ignore unsafe selector warning.

How:
The current implementation only compared the previous element, so all elements are now compared.

Checklist:

Documentation N/A
Tests
Code complete
Changeset

changeset-bot · 2022-05-17T13:20:55Z

🦋 Changeset detected

Latest commit: b82bcee

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

This PR includes changesets to release 1 package

Name	Type
@emotion/cache	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

srmagura

Nice work! This looks good to me, but let's wait for @Andarist's approval.

Can you add a changeset to record this patch-level change to @emotion/cache?

Edit: I noticed that CI is failing with

@emotion/cache "@babel/runtime/helpers/createForOfIteratorHelperLoose" is imported by "src/stylis-plugins.js" but the package is not specified in dependencies or peerDependencies

This is caused by the for..of loop you added. We still technically support IE 11, so we have to stick to old-school JS in some places. Can you refactor the for..of to a classic for loop or a .forEach()?

codesandbox-ci · 2022-05-17T23:37:42Z

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

Sandbox	Source
Emotion	Configuration

codecov · 2022-05-17T23:39:16Z

Codecov Report

Merging #2751 (b82bcee) into main (d39379c) will decrease coverage by 1.08%.
The diff coverage is 81.25%.

Impacted Files	Coverage Δ
packages/cache/src/stylis-plugins.js	`86.08% <81.25%> (-13.92%)`	⬇️
packages/serialize/src/index.js	`93.98% <0.00%> (-6.02%)`	⬇️
packages/react/src/global.js	`94.73% <0.00%> (-3.51%)`	⬇️
packages/react/src/class-names.js	`97.10% <0.00%> (-2.90%)`	⬇️

kyoncy · 2022-05-17T23:56:12Z

@srmagura
Thanks for reviewing!

Can you add a changeset to record this patch-level change to @emotion/cache?

Add a changeset commit!

Can you refactor the for..of to a classic for loop or a .forEach()?

I used classic for loop because it is returnable!

srmagura

Apart from making the changeset more descriptive, this looks good to me. The changesets end up in our release notes so they need to clearly explain what was changed.

.changeset/witty-frogs-hear.md

Co-authored-by: Sam Magura <srmagura@gmail.com>

kyoncy · 2022-05-24T12:36:31Z

@Andarist How is this going?

Andarist · 2022-05-24T12:46:31Z

packages/react/__tests__/__snapshots__/warnings.js.snap

@@ -52,6 +66,20 @@ exports[`unsafe pseudo classes does not warn when using with flag: /* emotion-di
 />
 `;

+exports[`unsafe pseudo classes does not warn when using with flag: /* emotion-disable-server-rendering-unsafe-selector-warning-please-do-not-use-this-the-warning-exists-for-a-reason */ ":first-child :nth-child(3) /* [flag] */" in a style multiple selectors 1`] = `


Based on the [flag] here it seems that the original intention was to replace that long string in test titles to aid the readability. There is .replace call somewhere that perhaps doesn't replace all occurrences of that flag in the input string - could you check if this assumption is correct and if we could fix this?

ah, actually the full flag is part of the describe's title and is literally passed in there:

emotion/packages/react/__tests__/warnings.js

Line 88 in 7a10769

describe(`does not warn when using with flag: ${ignoreSsrFlag}`, () => {

So we don't need to change anything here and that was the original intention (perhaps we could change it separately, but it doesn't matter)

There is .replace call somewhere that perhaps doesn't replace all occurrences of that flag in the input string

Replaced by /* [flag] */ here.

emotion/packages/react/__tests__/warnings.js

Lines 117 to 120 in d39379c

it(`"${pseudoClass.replace(

/\/\* \S+ \*\//g,

'/* [flag] */'

)}" in a style ${type}`, () => {

Do you need to modify or add test cases?

Andarist · 2022-05-24T12:49:16Z

packages/cache/src/stylis-plugins.js

+    for (let i = 0; i < children.length; i++) {
+      const element = children[i]
+      if (element && isIgnoringComment(last(element.children))) {
+        return
+      }
    }


Could you describe the change? What was the assumption of the previous code? How the fixed case is different and why it didn't work with the previous logic? What was the thought process behind the introduced change? I would appreciate if you could show the AST layouts of different cases so it would be easier to reason about this fix.

In the previous logic, only the first ignoreFlag in a style worked.
Therefore, CI passed, but if there were multiple styles and the second or later style used unsafe pseudo classes (ex. :first-child), an error would be displayed even if the ignoreFlag was given.

Andarist · 2022-05-24T12:49:31Z

Could you check if this would fix the recently reported issue: #2763 ?

kyoncy · 2022-08-18T11:28:16Z

Could you check if this would fix the recently reported issue: #2763 ?

@Andarist
Sorry for the late reply.
I applied the modifications and deployed the same code as in the codesandbox example!

deployed site: https://kyoncy.github.io/emotion-test/
repository: https://github.com/kyoncy/emotion-test/tree/main

Andarist · 2022-08-18T22:45:07Z

@kyoncy thanks for taking a look. I've pushed out some commits to make this comment detection more strict - we should be able to land this soon. I will just wait a couple of days to give a chance to other maintainers to review this

swernerx · 2022-08-23T14:11:16Z

Hi! I know this is not a fully valid bug report. Still it seems that updating from v11.10.1 to v11.10.2 breaks in our application. I receive the error: Cannot read properties of undefined (reading 'type') at isIgnoringComment2. Possibly element can be null/undefined?

swernerx · 2022-08-23T14:24:55Z

Seems to be related to:

var isIgnoringComment = function isIgnoringComment(element) {
  return element.type === 'comm' && element.children.indexOf(ignoreFlag) > -1;
};

which can be fixed using:

var isIgnoringComment = function isIgnoringComment(element) {
  return element && element.type === 'comm' && element.children.indexOf(ignoreFlag) > -1;
};

swernerx · 2022-08-23T14:34:31Z

The code involved producing the issue is:

  terminatedContract: {
    '& .contractIcon, tr:nth-child(1) td:nth-child(1), tr:nth-child(n+2) td': {
      opacity: 0.4
    }
  }

I guess it's related to the multiple usage of :nth-child. When I migrate that code to:

  terminatedContract: {
    '& .contractIcon, tr:nth-of-type(1) td:nth-of-type(1), tr:nth-of-type(n+2) td': {
      opacity: 0.4
    }
  }

the issue disappears.

Andarist · 2022-08-23T16:54:38Z

Thank you for the report - I've already merged in the fix for this and gonna release a new version in a second.

srmagura suggested changes May 17, 2022

View reviewed changes

srmagura requested a review from Andarist May 17, 2022 13:59

srmagura approved these changes May 18, 2022

View reviewed changes

.changeset/witty-frogs-hear.md Outdated Show resolved Hide resolved

kyoncy and others added 4 commits May 19, 2022 22:08

fix ignoreFlag not works

2ebf668

Add changeset

489488e

Fix for loop syntax

4093894

Update .changeset/witty-frogs-hear.md

7a10769

Co-authored-by: Sam Magura <srmagura@gmail.com>

kyoncy force-pushed the main branch from 9f86e34 to 7a10769 Compare May 19, 2022 13:08

Andarist reviewed May 24, 2022

View reviewed changes

kyoncy and others added 4 commits August 18, 2022 21:04

Merge branch 'main' into main

7ba9bbb

Refactor looping test to .each

9a53e9c

Fixed the issue with ignore comment detection

e2fdced

add changeset attributions

b82bcee

Andarist requested a review from emmatown August 18, 2022 22:43

Andarist requested a review from srmagura August 18, 2022 22:45

srmagura approved these changes Aug 22, 2022

View reviewed changes

Andarist merged commit 0ffd606 into emotion-js:main Aug 22, 2022

github-actions bot mentioned this pull request Aug 22, 2022

Version Packages #2860

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ignoreFlag not works bug #2751

Fix ignoreFlag not works bug #2751

kyoncy commented May 17, 2022 •

edited

Loading

changeset-bot bot commented May 17, 2022 •

edited

Loading

srmagura left a comment •

edited

Loading

codesandbox-ci bot commented May 17, 2022 •

edited

Loading

codecov bot commented May 17, 2022 •

edited

Loading

kyoncy commented May 17, 2022

srmagura left a comment

kyoncy commented May 24, 2022

Andarist May 24, 2022

Andarist May 24, 2022

kyoncy Aug 18, 2022

Andarist May 24, 2022

kyoncy Aug 18, 2022

Andarist commented May 24, 2022

kyoncy commented Aug 18, 2022 •

edited

Loading

Andarist commented Aug 18, 2022

swernerx commented Aug 23, 2022

swernerx commented Aug 23, 2022 •

edited

Loading

swernerx commented Aug 23, 2022

Andarist commented Aug 23, 2022

	it(`"${pseudoClass.replace(
	/\/\* \S+ \*\//g,
	'/* [flag] */'
	)}" in a style ${type}`, () => {

Fix ignoreFlag not works bug #2751

Fix ignoreFlag not works bug #2751

Conversation

kyoncy commented May 17, 2022 • edited Loading

changeset-bot bot commented May 17, 2022 • edited Loading

🦋 Changeset detected

srmagura left a comment • edited Loading

Choose a reason for hiding this comment

codesandbox-ci bot commented May 17, 2022 • edited Loading

codecov bot commented May 17, 2022 • edited Loading

Codecov Report

kyoncy commented May 17, 2022

srmagura left a comment

Choose a reason for hiding this comment

kyoncy commented May 24, 2022

Andarist May 24, 2022

Choose a reason for hiding this comment

Andarist May 24, 2022

Choose a reason for hiding this comment

kyoncy Aug 18, 2022

Choose a reason for hiding this comment

Andarist May 24, 2022

Choose a reason for hiding this comment

kyoncy Aug 18, 2022

Choose a reason for hiding this comment

Andarist commented May 24, 2022

kyoncy commented Aug 18, 2022 • edited Loading

Andarist commented Aug 18, 2022

swernerx commented Aug 23, 2022

swernerx commented Aug 23, 2022 • edited Loading

swernerx commented Aug 23, 2022

Andarist commented Aug 23, 2022

kyoncy commented May 17, 2022 •

edited

Loading

changeset-bot bot commented May 17, 2022 •

edited

Loading

srmagura left a comment •

edited

Loading

codesandbox-ci bot commented May 17, 2022 •

edited

Loading

codecov bot commented May 17, 2022 •

edited

Loading

kyoncy commented Aug 18, 2022 •

edited

Loading

swernerx commented Aug 23, 2022 •

edited

Loading