From b9b8b7493d6aea7cb2543aa1925eb2cac9636be9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Burzy=C5=84ski?= Date: Tue, 23 Aug 2022 18:53:44 +0200 Subject: [PATCH] Fixed a regression that could cause a crash when looking for an ignoring comment for unsafe pseudo-classes (#2864) * Fixed a regression that could cause a crash when looking for an ignoring comment for unsafe pseudo-classes * Fix flow types --- .changeset/polite-points-study.md | 5 + packages/cache/src/stylis-plugins.js | 12 +- .../__tests__/__snapshots__/warnings.js.snap | 58 ++++++++ packages/react/__tests__/warnings.js | 125 +++++++++++++++++- 4 files changed, 192 insertions(+), 8 deletions(-) create mode 100644 .changeset/polite-points-study.md diff --git a/.changeset/polite-points-study.md b/.changeset/polite-points-study.md new file mode 100644 index 000000000..92c75b76d --- /dev/null +++ b/.changeset/polite-points-study.md @@ -0,0 +1,5 @@ +--- +'@emotion/cache': patch +--- + +Fixed a regression that could cause a crash when looking for an ignoring comment for unsafe pseudo-classes. diff --git a/packages/cache/src/stylis-plugins.js b/packages/cache/src/stylis-plugins.js index 43bac0cb7..e0fc8f243 100644 --- a/packages/cache/src/stylis-plugins.js +++ b/packages/cache/src/stylis-plugins.js @@ -186,15 +186,15 @@ export let createUnsafeSelectorsAlarm = cache => (element, index, children) => { : // global rule at the root level children - for (let i = 0; i < commentContainer.length; i++) { + for (let i = commentContainer.length - 1; i >= 0; i--) { const node = commentContainer[i] - if (node.line > element.line) { + if (node.line < element.line) { break } // it is quite weird but comments are *usually* put at `column: element.column - 1` - // so we seek for the node that is later than the rule's `element` and check the previous element + // so we seek *from the end* for the node that is earlier than the rule's `element` and check that // this will also match inputs like this: // .a { // /* comm */ @@ -209,10 +209,8 @@ export let createUnsafeSelectorsAlarm = cache => (element, index, children) => { // } // with such inputs we wouldn't have to search for the comment at all // TODO: consider changing this comment placement in the next major version - if (node.column > element.column) { - const previousNode = commentContainer[i - 1] - - if (isIgnoringComment(previousNode)) { + if (node.column < element.column) { + if (isIgnoringComment(node)) { return } break diff --git a/packages/react/__tests__/__snapshots__/warnings.js.snap b/packages/react/__tests__/__snapshots__/warnings.js.snap index 5bd0f885c..3b9bb460a 100644 --- a/packages/react/__tests__/__snapshots__/warnings.js.snap +++ b/packages/react/__tests__/__snapshots__/warnings.js.snap @@ -90,6 +90,64 @@ 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 */ does warn when not using the flag on a global rule 1`] = `null`; + +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 */ does warn when not using the flag on the rule that follows a declaration 1`] = ` +.emotion-0 { + color: hotpink; +} + +.emotion-0>*:first-child { + margin-left: 0; +} + +
+`; + +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 */ does warn when not using the flag on the rule that follows another rule 1`] = ` +.emotion-0>* { + margin-left: 10px; +} + +.emotion-0>*:first-child$ { + margin-left: 0; +} + +
+`; + +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 */ does warn when not using the flag on the rule that preceeds a declaration 1`] = ` +.emotion-0 { + color: hotpink; +} + +.emotion-0>*:first-child { + margin-left: 0; +} + +
+`; + +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 */ does warn when not using the flag on the rule that preceeds another rule 1`] = ` +.emotion-0>*:first-child { + margin-left: 0; +} + +.emotion-0>* { + margin-left: 10px; +} + +
+`; + 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 */ with object styles :first-child :nth-child(3) 1`] = ` .emotion-0:first-child :nth-child(3) { color: rebeccapurple; diff --git a/packages/react/__tests__/warnings.js b/packages/react/__tests__/warnings.js index 5454788cc..f92343258 100644 --- a/packages/react/__tests__/warnings.js +++ b/packages/react/__tests__/warnings.js @@ -126,6 +126,129 @@ describe('unsafe pseudo classes', () => { }) }) + test('does warn when not using the flag on the rule that follows another rule', () => { + expect( + renderer + .create( +
*': { + marginLeft: 10 + }, + [`& > *:first-child$`]: { + marginLeft: 0 + } + }} + /> + ) + .toJSON() + ).toMatchSnapshot() + expect((console.error: any).mock.calls).toMatchInlineSnapshot(` + [ + [ + "The pseudo class ":first-child" is potentially unsafe when doing server-side rendering. Try changing it to ":first-of-type".", + ], + ] + `) + }) + + test('does warn when not using the flag on the rule that preceeds another rule', () => { + expect( + renderer + .create( +
*:first-child`]: { + marginLeft: 0 + }, + '& > *': { + marginLeft: 10 + } + }} + /> + ) + .toJSON() + ).toMatchSnapshot() + expect((console.error: any).mock.calls).toMatchInlineSnapshot(` + [ + [ + "The pseudo class ":first-child" is potentially unsafe when doing server-side rendering. Try changing it to ":first-of-type".", + ], + ] + `) + }) + + test('does warn when not using the flag on the rule that follows a declaration', () => { + expect( + renderer + .create( +
*:first-child`]: { + marginLeft: 0 + } + }} + /> + ) + .toJSON() + ).toMatchSnapshot() + expect((console.error: any).mock.calls).toMatchInlineSnapshot(` + [ + [ + "The pseudo class ":first-child" is potentially unsafe when doing server-side rendering. Try changing it to ":first-of-type".", + ], + ] + `) + }) + + test('does warn when not using the flag on the rule that preceeds a declaration', () => { + expect( + renderer + .create( +
*:first-child`]: { + marginLeft: 0 + }, + color: 'hotpink' + }} + /> + ) + .toJSON() + ).toMatchSnapshot() + expect((console.error: any).mock.calls).toMatchInlineSnapshot(` + [ + [ + "The pseudo class ":first-child" is potentially unsafe when doing server-side rendering. Try changing it to ":first-of-type".", + ], + ] + `) + }) + + test('does warn when not using the flag on a global rule', () => { + expect( + renderer + .create( + *:first-child`]: { + marginLeft: 0 + } + }} + /> + ) + .toJSON() + ).toMatchSnapshot() + expect((console.error: any).mock.calls).toMatchInlineSnapshot(` + [ + [ + "The pseudo class ":first-child" is potentially unsafe when doing server-side rendering. Try changing it to ":first-of-type".", + ], + ] + `) + }) + test('does not warn when using the flag on the rule that follows another rule', () => { expect( renderer @@ -184,7 +307,7 @@ describe('unsafe pseudo classes', () => { expect(console.error).not.toBeCalled() }) - test.only('does not warn when using the flag on the rule that preceeds a declaration', () => { + test('does not warn when using the flag on the rule that preceeds a declaration', () => { expect( renderer .create(