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.

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

Cannot turn off "potentially unsafe when doing server-side rendering" noise #1105

Closed
RoystonS opened this issue Dec 13, 2018 · 64 comments
Closed

Comments

@RoystonS
Copy link
Contributor

Emotion 10 is emitting a series of console warnings along the lines of:

The pseudo class ":first-child" is potentially unsafe when doing server-side rendering. Try changing it to "first-of-type"
The pseudo class ":nth-child" is potentially unsafe when doing server-side rendering. Try changing it to "nth-of-type"
  • emotion version: 10.0.4
  • react version: 16.7.0-alpha2

Problem description:

  1. I'm not using server-side rendering so these warnings aren't useful for me and are just console noise that means real problems will be masked. Apart from running in a production build, I can't see any way to turn these warnings off right now.
  2. The error message doesn't help me understand why these pseudo classes are a problem; I can't find them mentioned in the Emotion docs, or references on the Internet as to why these might be a problem.

Suggested solution:

  1. Provide a way to turn off these warnings if users don't want them.
  2. Provide a link, or some documentation, as to why these pseudos are problematic.
@Andarist
Copy link
Member

Leaving this here - #1059 (comment) as this issue is dedicated to this problem.

krazijames pushed a commit to ridi/design-system that referenced this issue Dec 27, 2018
@vajkri
Copy link

vajkri commented Jan 8, 2019

We are getting the warnings too using SSR.
The styles targeting with :nth-child(), :first-child or :last-child don't even get applied server-side, so this causes elements to "jump" on page load when these styles get applied client-side.

krazijames added a commit to ridi/design-system that referenced this issue Jan 15, 2019
* Implement Tabs component

* Fix `:first-child` warning

emotion-js/emotion#1105

* Fix font size

* Add active color example

* Implement flex option

* Fix focus style

* Change style file's extension

* Add combined example

* Make anchor example work

* Extract item wrapper style from tabs style

* Remove combined example

* Rename flex to stretchItems
@brentertz
Copy link
Contributor

brentertz commented Jan 24, 2019

Hi Folks,

The inability to disable these console messages is a blocker for us being able to ship the Emotion 10 update to our component library. As a library, these messages are also visible to all of our downstream consumers.

We do not wish to update the selector usage in our library as we do not believe Emotion's current SSR strategy to be a valid trade-off for breaking numerous commonly used CSS selectors, hence #1178. We are hopeful that an alternate approach can be found there as well, but in the mean time, simply would like to disable these console messages.

Is there anything that can be done here to move things forward? We are willing to do the work, or collaborate, if you can provide some insight into the direction that you would like to take.

Possible Solutions

  • Simply remove logging; instead improve and rely on SSR documentation
  • Conditionally remove logging, e.g. ENV var or other opt out mechanism
  • Only log when server side rendering
  • Only log message once per page load
    • Unsure if this would avoid excess logging during unit tests though
  • Remove the Stylis “linting” plugins from the default cache implementation and make them opt-in
  • Use linter/stylelint instead
    • Rules then become user configurable as ignored, warning, or error
    • Create and document a stylelint-config-emotion package
  • Adjust the Stylis "linting" plugin to support a special style property that allows the message to be suppressed. See PR [cache] Add ignore flag for the unreliable selectors warning #1209

Additional Considerations

  • Our product is an open-source component library, as opposed to an application, so preferably any solution will not place an additional configuration burden on downstream consumers.

Lastly, thank you for all of your efforts in creating and maintaining this project, which solves a lot of our problems!

@brentertz
Copy link
Contributor

@Andarist Do you have any thoughts on my comments above?

@Andarist
Copy link
Member

Well, personally I think the story around this has to be improved somehow. There is somewhat more fresh discussion around this here. Could you take a look there and comment what do you think about stuff mentioned there?

@brentertz
Copy link
Contributor

brentertz commented Feb 1, 2019

Hi @Andarist Thank you for your attention.

I am aware of that issue and even link to it in my comments above. I'd be happy to comment on it further following this message. For full transparency, @kylegach and I work together.

Whilst Emotion's SSR implementation is certainly at the heart of this issue, I would imagine that is a bigger issue to resolve and I did not want to presume that you would be open to making changes in that area.

In the mean time, I believe that finding a way to resolve this issue would provide immediate value to many Emotion users, particularly those affected that aren't even doing SSR. I understand not wanting to put a lot of work into it if changing the underlying implementation could just make this issue go away, but what if it could be resolved with minimal effort and minimal impact?

I recently added another possible solution to the list in my comment above, which feels pretty lightweight. I'll reiterate it here.

@Andarist
Copy link
Member

Andarist commented Feb 2, 2019

Unfortunately this is tightly coupled to SSR concerns - especially for your use case. From what I understand you develop a UI library which uses emotion and ideally your consumers shouldn't be aware of emotion and they should be able to do SSR without suffering the FOUC.

@brentertz
Copy link
Contributor

I agree that this issue should ideally be solved by altering Emotion's SSR implementation to not “break” standard CSS, but I don’t believe that issue will be resolved in as timely a manner as this one may be mitigated. Again, I think #1209 resolves the issue quite minimally.

@RoystonS
Copy link
Contributor Author

Whilst #1209 is a 'minimal' change to emotion, it's not a 'minimal' change for a user: it requires each use of a :first-child selector to be flagged with a really long "emotion-disable-server-rendering-unsafe-selector-warning-please-do-not-use-this-the-warning-exists-for-a-reason" string. (This would bloat my output bundles in a rather unpleasant way.)

Isn't it the environment (SSR or not) that determines whether those selector usages are safe or not, rather than the individual uses? e.g. I'm using this in an environment that goes nowhere near SSR, so I'd much rather just tell emotion/stylis/cache/whatever once (not once per use) that it's safe to use these selectors.

@brentertz
Copy link
Contributor

brentertz commented Mar 12, 2019

@RoystonS I agree with you. We had to work within the constraints of what the maintainers would allow. IMHO, #1209 is a workaround and the real way to solve this issue is to not break standard CSS.

FWIW, you can always assign the string to a variable. Also, the obnoxiously long comment is stripped from the build, so it should not affect your bundle size at all.

@RoystonS
Copy link
Contributor Author

Yeah, I started putting it into a var, but then realised it was simpler just to adjust our build process to patch @emotion/cache (before we run webpack) to remove the warning. Not really an option for library authors, of course...

You mentioned the comment being stripped from the build - what would do that? It's a comment inside a string, so a minifier wouldn't know it can be removed... (And there's another copy inside @emotion/cache itself. :-( so my hack-the-package before build wins on both counts. :-) )

@brentertz
Copy link
Contributor

@RoystonS My mistake, it feels like ages since I worked on this issue. The strings do remain in the bundle, though the effect could likely be minimized using process.env.NODE_ENV. It sounds like you are using a better solution for non-library authors anyway. :)

@ahutchings
Copy link
Contributor

ahutchings commented Mar 27, 2019

You mentioned the comment being stripped from the build - what would do that? It's a comment inside a string, so a minifier wouldn't know it can be removed...

My team is running into this issue. babel-plugin-emotion auto-minifies emotion CSS, stripping comments in the process. There doesn't appear to be an option to disable stripping comments, so unfortunately the comments to suppress the "unsafe selector" noise only work when not using the babel plugin (likely only in development). My team is using the babel plugin in tests for autolabeling and source maps, so we're seeing the warnings and there is no apparent way to suppress them. Until the larger issue is resolved (#1178), there are a few ways to mitigate this:

  1. Only check for and warn about unsafe selectors in development (some sort of flag/option for @emotion/cache, environment variable, etc). Maybe less code could be shipped to prod this way?
  2. Add an option to babel-plugin-emotion to disable stripping all comments.
  3. Update babel-plugin-emotion to be aware of the "unsafe selector" comment and leave it in place automatically.

@mitchellhamilton @kylegach Do either of you have any thoughts around this? We may be able to contribute something for this via PR, but would like to have consensus on the direction before opening one.

@kylegach
Copy link
Contributor

@ahutchings — The code, both to check and to warn, is wrapped in a not-prod check (line 174), so (1) is already the case.

I'll let Mitchell weigh in on the other points.

For clarification, I slightly misinterpreted Royston's comment earlier. I meant to say that the suppress comment is stripped in the output CSS; he is correct that it is still in the bundle itself (unless running babel-plugin-emotion, as you've discovered).

@ahutchings
Copy link
Contributor

@kylegach Thanks, I missed that check at the top. Do you have a suggestion for how we can use the suppression comment when running unit tests in Jest (along with the babel plugin), where NODE_ENV is set to "test" automatically by the runner?

We're also developing a component library, and would like for our users to be able to take advantage of emotion source maps (provided by the babel plugin) in development without seeing the unsafe selector errors, but I suppose that is more the larger issue that needs to be resolved (#1178).

@kylegach
Copy link
Contributor

@ahutchings — I don't, and I'm fairly sure a fix would require a code change in either the babel plugin or (somehow) the cache package. We don't run the plugin in our tests, so we missed this issue. Sorry about that.

Agreed that addressing the larger issue of having unreliable selectors at all would be a better long-term solution.

@kylegach
Copy link
Contributor

@ahutchings — I think this may be more nuanced than we've discussed so far. Here's a codesandbox running our components, which have multiple unreliable selectors (e.g. in Button) and are using the plugin (via babel-preset-css-prop, which includes babel-plugin-emotion). If you open the console, you can see that it's not running in production mode, but there are no selector warnings logged.

I think this means it's at least possible to get the desired behavior in a test environment, but I'm not sure what about your setup is causing the issue to be sure.

@ahutchings
Copy link
Contributor

@kylegach Thanks for your prompt responses and for looking into it further. The "ignore" comments are still in place in the version of mineral-ui pulled down in the sandbox, so the warning is not logged. The babel plugin may be leaving the "ignore" comments in place due to the way the comments are defined in mineral-ui source (object-based styles with a computed property + reference to the ignore string).

js _objectSpread2['& [role="img"]:first-child' + _emotion.ignoreSsrWarning]

We are using the tagged-template-literal-based method of defining styles in our components library, and the comments are stripped out whether we define them inline or using a reference to the "ignoreSsrWarning" variable like mineral-ui does.

@kylegach
Copy link
Contributor

@ahutchings — Ahh, the meaningful difference is the object styles vs. tagged template literal styles. I'm unsure how to proceed from here, but I suggest starting with a new, dedicated issue to keep the conversation focused going forward. It still seems to me like the plugin code would need changed for the fix. I know stylis, which emotion uses to parse styles, already strips comments before outputting CSS, so it's redundant (and problematic) for the plugin to do the same.

@nelsonlaquet
Copy link

This is a pretty serious limitation. I understand it has to do with <style> elements being added inline in the DOM when doing SSR; is it at all possible for emotion to... not do that?

I've been using emotion for a whole of a week and have ran into this issue multiple times, where using :first-of-type isn't a valid substitute. To get around this issue I've had to add superfluous

elements, which clutter the DOM unnecessarily, and complicate the use of my components.

@antoinerousseau
Copy link

Anyone knows if this happens using styled-components too? Or is it emotion-specific?

@codephunk
Copy link

codephunk commented Jul 31, 2021

Hey everyone, I just figured out a dirty way of getting rid of this irritating warning.

Thank you! Works like a charm.
But i think the first if should look like this, otherwise the result will always be false.

if (process.env.NODE_ENV !== "development") {

@roeycohen
Copy link

hi @seafoam6, the string check should be args[0]

zdavis added a commit to ManifoldScholar/manifold that referenced this issue Nov 16, 2021
This commit introduces a webpack rule that monkey patches an Emotion
warning. We're using Emotion's "advanced" approach to SSR, which means
that the styles are inserted in the head, rather than before each
component. The first child-warning, as we understand it, exists to warn
developers that the insertion of the style tag could change which
element is the first-child, which isn't an issue in our case. Sadly,
emotion doesn't provide a viable way to disable this warning, so we're
monkey patching it here to stop it from spamming the console.

See emotion-js/emotion#1105
zdavis added a commit to ManifoldScholar/manifold that referenced this issue Nov 17, 2021
This commit introduces a webpack rule that monkey patches an Emotion
warning. We're using Emotion's "advanced" approach to SSR, which means
that the styles are inserted in the head, rather than before each
component. The first child-warning, as we understand it, exists to warn
developers that the insertion of the style tag could change which
element is the first-child, which isn't an issue in our case. Sadly,
emotion doesn't provide a viable way to disable this warning, so we're
monkey patching it here to stop it from spamming the console.

See emotion-js/emotion#1105
zdavis added a commit to ManifoldScholar/manifold that referenced this issue Nov 17, 2021
This commit introduces a webpack rule that monkey patches an Emotion
warning. We're using Emotion's "advanced" approach to SSR, which means
that the styles are inserted in the head, rather than before each
component. The first child-warning, as we understand it, exists to warn
developers that the insertion of the style tag could change which
element is the first-child, which isn't an issue in our case. Sadly,
emotion doesn't provide a viable way to disable this warning, so we're
monkey patching it here to stop it from spamming the console.

See emotion-js/emotion#1105
zdavis added a commit to ManifoldScholar/manifold that referenced this issue Nov 17, 2021
This commit introduces a webpack rule that monkey patches an Emotion
warning. We're using Emotion's "advanced" approach to SSR, which means
that the styles are inserted in the head, rather than before each
component. The first child-warning, as we understand it, exists to warn
developers that the insertion of the style tag could change which
element is the first-child, which isn't an issue in our case. Sadly,
emotion doesn't provide a viable way to disable this warning, so we're
monkey patching it here to stop it from spamming the console.

See emotion-js/emotion#1105
@Mario-Eis
Copy link

Mario-Eis commented Jan 24, 2022

This is closed, but what is the solution for that. We render content. And we do not know, what content will be displayed. But we need the first and the last element to have no margins. Regardless of its type. What's the solution?

@coolassassin
Copy link

coolassassin commented Feb 4, 2022

Hey everyone, I just figured out a dirty way of getting rid of this irritating warning.

Hello, I have tried this code and it breaks all console.error calls.
And I can explain you why.
The argument args is an array, but you work with it like with string.
Also using indexOf as is, it is a potential bug, which we can see here.
If something not matching the case we have -1 in both cases and !!(-1 && -1) === true

So I see comments bellow about your mistakes, but you didn't fix them.
So, that is the right one to avoid mistakes for guys like me:

export const disableEmotionWarnings = () => {
  if (!process.env.NODE_ENV !== 'development') {
    return;
  }
  const log = console.error.bind(console);
  console.error = (...args) => {
  if (
    typeof args[0] === 'string' &&
    args[0].includes('The pseudo class') &&
    args[0].includes('is potentially unsafe when doing server-side rendering. Try changing it to')
  ) {
    return;
  }
  log(...args);
  };
};

@Andarist
Copy link
Member

Andarist commented Mar 3, 2022

You can also disable those warnings by setting .compat = true on a cache.

@caedney
Copy link

caedney commented May 13, 2022

For anyone still experiencing this problem, I was able to work around this issue by essentially recreating :first-child with :first-of-type :

& > :first-of-type:not(style):not(:first-of-type ~ *),
& > style + * {
    margin-top: 0;
}

Hope this helps.

@Nantris
Copy link
Contributor

Nantris commented Jun 29, 2022

@Andarist is there no way to disable this without the need for a custom cache? It's rather onerous.

@Nantris
Copy link
Contributor

Nantris commented Jul 1, 2022

Fyi @caedney that style rule is going to be quite taxing on the CPU compared to :first-child.

I think that a simpler way to disable this should be a goal for Emotion 12.

@Schnueggel
Copy link

Its really funny that after all this years there is no real solution to this.

@kinoli
Copy link

kinoli commented Aug 27, 2022

You can check if you are in Jest and only include the pseudo code if you are not in jest. This works for me.

${!process.env.JEST_WORKER_ID
      ? css`
          *:first-child {
            margin-top: 0;
          }

          *:last-child {
            margin-bottom: 0;
          }
        `
      : null}

jamesricky added a commit to vivid-planet/comet that referenced this issue Aug 29, 2022
This is to suppress irrelevant but annoying error messages in the
console about potential issues with server-side rendering with emotion.

There is no other way to disable these error messages, even though we
only use client-side rendering: emotion-js/emotion#1105.
@Hideman85
Copy link

Do we have a solution for this really really annoying log? We are not concerned at all about SSR today and we are annoyed all the time with this "Warning" that is even an error 🤦‍

Please can I mute this at the source? Thanks 🙏

@allpro
Copy link

allpro commented Sep 17, 2023

I appreciate the emotion library and the developers who put so much work into it. However, it is time all library authors learn that it is up to developers to decide their project's coding practices. It is arrogant for a 'library' to presume it knows best for every use-cases. This is a trend that needs to stop. The many complaints in this thread, over a 5 year period, illustrate how highly annoying this is.

Such 'hints' are useful, but there should be easy-to-use options. The ideal scenario is granular control, such as eslint-style rules, so that developers get to decide which warnings are appropriate and which are not. Ideally the severity (warning vs error) could also be configured. The message in this thread notes that it is only "potentially unsafe", yet it still shouts at devs as a console 'warning'. At a minimum this should only be a 'warning'.

This non-applicable error caused some of my devs to change the accurate and superior use of :first-child with :first-of-type. This made the CSS less clear, and in at one case created a bug. Teaching the developers in my team how to write CSS is my job, not this library's.

Again, thanks to all the maintainers of this library. However as the years of feedback above indicates, a more refined approach to such messaging would be a great improvement. I'm glad I found the compat option here, but this seems like a sledgehammer solution to a nuanced problem. It also seems this is still too hard to find for many users.

@christiaanwesterbeek
Copy link

If I understand correctly I need to create an EmotionCacheProvider and wrap my app with it. I am afraid the app will end up with 2 caches. This one and some internal one by MUI. Since I have no idea what the Emotion Cache is and I don't want to dive into it, I add the following to my app which silences the error also.

const consoleError = console.error;

console.error = function filterErrors(msg, ...args) {
    if (/server-side rendering/.test(msg)) {
        return;
    }
    consoleError(msg, ...args);
};

Copied from here oliviertassinari/react-swipeable-views#534 (comment) and adjusted...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests