facebook / create-react-app

Set up a modern web app by running one command.
https://create-react-app.dev
MIT License
102.3k stars 26.72k forks source link

CSS custom properties are not polyfilled #7185

Open C-Higgins opened 5 years ago

C-Higgins commented 5 years ago

Is this a bug report?

Yes

I have CSS variables (custom properties) in my CSS, which are not supported by IE. The CRA docs state "Support for new CSS features like the all property, break properties, custom properties, and media query ranges are automatically polyfilled to add support for older browsers." However, they are not polyfilled when they are imported from another file.

Steps to reproduce

// A.css
:root {
  --test: #fff;
}
// B.css
@import 'A.css'
body {
  color: var(--test)
}

Expected Behavior

The built CSS file should have the CSS variables processed out and replace the var(--) calls with the proper values

Actual Behavior

The autoprefixer works as expected, adding vendor prefixes and such, and changing the browserslist settings do alter the outputted CSS in some way. However, no browserslist setting is making the custom properties be post-processed, and as such the CSS will not work on IE. It seems like the variable replacement is happening before the concatenation from imports, and so it doesn't find the property definition from other files.

Note that I also deleted node_modules and package-lock and the issue persists.

jeromeraffin commented 5 years ago

Same problem, in the webpack config the postcss-preset-env is used at stage 3:

plugins: () => [
   require('postcss-flexbugs-fixes'),
   require('postcss-preset-env')({
     autoprefixer: {
     flexbox: 'no-2009',
    },
    stage: 3,
   }),
  ...

So I guess it should process and polyfill the custom properties (https://preset-env.cssdb.org/features#custom-properties)?

Or does the postcss custom properties plugin has to be added to the config in order to work? (https://github.com/postcss/postcss-custom-properties/blob/master/INSTALL.md#webpack).

hiramhibbard commented 5 years ago

I’m seeing this same issue.

chce commented 5 years ago

Also seeing this issue.

jeromeraffin commented 5 years ago

I tried to use the PostCSS plugin with craco because I don't want to eject the config of CRA but it did not give anything good, still have the same issue, is anyone has a solution?

jakke-korpelainen commented 4 years ago

Same issue here. Wouldn't want to eject or add deps by using 3rd party overrides.

mrmckeb commented 4 years ago

Unfortunately, there's no bulletproof way to polyfill these... and adding this could introduce issues.

I personally feel that you shouldn't use this feature unless you're only targeting browsers that support it.

hiramhibbard commented 4 years ago

@mrmckeb but isn't that what https://www.npmjs.com/package/postcss-css-variables is meant to do? The issue isn't whether or not a pollyfill can be bulletproof, but that CRA doesn't seem to be applying this package properly when building.

ivanmccarthy commented 4 years ago

Unfortunately, there's no bulletproof way to polyfill these... and adding this could introduce issues.

I personally feel that you shouldn't use this feature unless you're only targeting browsers that support it.

If this is the case, then I feel like it should be clearly stated in the documentation. The current docs read as if these polyfills are 100% supported.

mrmckeb commented 4 years ago

@ifmcrthy, can you point me to that part of the docs?

@hiramhibbard - adding this would actually introduce issues IMO. The problem is that it does not and cannot support scope - which is a huge part of CSS variables (custom properties).

To be clear, this is not a polyfill, but a static transform that happens at build time. This feature cannot be polyfilled, and it's very sad to me as I think it's an amazing feature.

This is the section I'm referring to - https://www.npmjs.com/package/postcss-css-variables#caveats.

I've tried to use this myself in production and we ended up having to educate developers on the caveats of the polyfill, to the point where we were just using it as a dumbed-down Sass variable.

chce commented 4 years ago

@mrmckeb https://facebook.github.io/create-react-app/docs/post-processing-css "Support for new CSS features like the all property, break properties, custom properties, and media query ranges are automatically polyfilled to add support for older browsers."

As for the caveat, it only refers to setting the value of the CSS variable outside :root, but we want to be able to refer to a CSS variable outside of :root, which should definitely be 100% possible in postcss-css-variables.

I don't agree that this is just a dumbed down Sass-variable and that argument misses the point IMO. I'm trying to get this transformation(once it correctly transforms the CSS variables) to work in collaboration with CSS ponyfill(https://www.npmjs.com/package/css-vars-ponyfill) to deal with changing the variables at runtime.

mrmckeb commented 4 years ago

Tnanks @chce, I'll ask the team now why that's listed but not supported. We might need to update the docs.

My point is this. Imagine you build Button, a reusable component. It has styles like this:

.Button {
   background-color: var(--button-background, blue);
}

Now, if --button-background is set in root, no problem.

If you then pass it into a component that does this (in a separate file):

.ParentComponent {
  --button-background: red;
}

That would work in Chrome perfectly. In any modern browser, that works. It's one of the core features of CSS Custom Properties.

In our app, it would completely fail. Which would be very confusing for developers...

chce commented 4 years ago

I accept that setting a variable in a selector isn't supposed to work, but this is becoming an argument of whether or not to have this polyfill at all. The point is that in the current state of create-react-app, the sample .Button-selector wouldn't even work if the variable is set in a different file, even if the file containing the variables is imported correctly.

mrmckeb commented 4 years ago

Sorry @chce, this is not meant to be an argument. I was only explaining why I wasn't a fan of this transform in general.

For reference, can you confirm:

  1. That this is occurring in production builds, not just development.
  2. Your current browserslist: NODE_ENV=production npx browserslist
jeromeraffin commented 4 years ago

@mrmckeb The problem is also happening when you defined variables in the :root, in my case what I tried is to defined a style.css file with the :root and then to import this stylesheet in the css module file of my component, but it's not working :/

chce commented 4 years ago
  1. tl;dr It works with .scss(both production and development), doesn't work with .css(both production and development). We've recently moved to Sass on my current project (due to issues with this polyfill), and completely went away from using CSS variables. I just tried to add a CSS variable(--test: green;) in a .scss file(test.scss), then import the file into a separate .scss module(Test.module.scss) and reference the variable(background-color: var(--test);), and it seems to polyfill it just fine now. Doing the same steps with CSS files doesn't seem to polyfill correctly in neither development nor production builds
  2. "browserslist": { "production": [ ">0.2%", "not dead", "not op_mini all" ], "development": [ "last 1 chrome version", "last 1 firefox version", "last 1 safari version", "last 1 edge version", "ie 11" ] },

    Can someone else check/confirm that the polyfill works if you use .scss?

jakke-korpelainen commented 4 years ago

Some additional reference data of a similar issue scenario. This is happening for us in development and production.

"browserslist": [ "> 1%", "ie 11" ]

$ NODE_ENV=prod npx browserslist and_chr 75 and_uc 11.8 chrome 75 chrome 74 edge 17 firefox 67 ie 11 ios_saf 12.2-12.3 ios_saf 12.0-12.1 op_mini all safari 12.1 samsung 9.2

hiramhibbard commented 4 years ago

@mrmckeb to reiterate what @jeromeraffin said, I'm not trying to define these variables in anything but :root and yet it doesn't work. I get what you're trying to say, but the fact is it doesn't work for :root either.

I feel like this discussion is distracting away from the fact that CRA states that this feature is supported, when in fact it does not currently work properly.

jakke-korpelainen commented 4 years ago

@chce switched to scss and can confirm that the polyfills are working.

jeromeraffin commented 4 years ago

@chce switched to scss too and it works perfectly, thank you for the temporary solution :)

vivekweb2013 commented 4 years ago

This is still not resolved? CSS custom properties are such a cool feature which I'm still not using due to its incompatibility with IE 11. I don't want to add SASS/LESS into the project, rather use this awesome CSS feature that is now part of CSS specification. Waiting for this issue to get resolved...

jzhen1603 commented 4 years ago

adding the plugin "postcss-easy-import" worked for me!

RoroTitiFR commented 4 years ago

I second all that have been said, I encounter this specific issue as well.

lodybo commented 4 years ago

@jzhen1603 Did you have to eject before adding that plugin? It doesn't work for me at the moment. We were also counting on having IE11 support for custom properties defined in :root and used in our project with @import, but no dice.

jzhen1603 commented 4 years ago

@lodybo , I am not sure what you mean by eject. What I did is that I added the plugin in package.json, put it in my webpack like below and did a rebuild: plugins: () => [ require('postcss-easy-import'), require('postcss-preset-env')({ stage: 3, }), ...

lodybo commented 4 years ago

@jzhen1603 I meant to ask if you ejected out of CRA to customize the webpack configuration? Normally the webpack config is abstracted away by CRA. I was curious how you configured it to work.