remix-run / remix

Build Better Websites. Create modern, resilient user experiences with web fundamentals.
https://remix.run
MIT License
29.18k stars 2.46k forks source link

esbuild minifies CSS in a way that can break in old browsers #6114

Open 19Qingfeng opened 1 year ago

19Qingfeng commented 1 year ago

What version of Remix are you using?

@remix-run/dev 1.15.0

Are all your remix dependencies & dev-dependencies using the same version?

Steps to Reproduce

I found this compilation of styles.

image

when i use postcss-preset-env compile my style code, eg: body { position: fixed; inset: 0; }

First, it will be processed by postcss,get body { position: fixed; top:0; bottom:0; left:0; right:0; }.

Then esbuild will process my style code,This will translate into body { position: fixed; inset:0; }

Expected Behavior

Keep The postcss-preset-env dealing with me.

Actual Behavior

Now, then esbuild will process my css code after then postcss.

machour commented 1 year ago

Hey @19Qingfeng, it would help a lot if you could provide a repository reproducing the problem.

markdalgleish commented 1 year ago

I had a look into this. It's not strictly related to PostCSS because you can manually write a CSS file containing your example code (body { position: fixed; top:0; bottom:0; left:0; right:0; }) and get the same result.

This is more of an issue with esbuild's CSS minification logic which is quite aggressive by default. I was able to fix it for CSS file imports by setting the targets array on the build inside cssImportsPlugin. I tried setting it to the list of browser versions needed for native JS module support since that's the absolute baseline for Remix:

target: [
  "chrome61",
  "edge16",
  "safari11",
  "ios11",
  "firefox60",
  "opera48",
],

This worked for CSS file imports (i.e. import href from "./styles.css") but this threw errors during our CSS bundle build since that also bundles JS, and esbuild can't handle certain JS features when targeting these browser versions.

I can fix this issue (at least for the default remix template) by raising the browser versions to the following:

target: [
  "chrome63",
  "edge79",
  "safari12",
  "ios12",
  "firefox60",
  "opera50",
],

However, this feels a bit brittle to me since we can't decouple CSS and JS minification options and we might run into further issues with other JS features. I think the safer approach would be to look into a custom CSS minification step in our compiler (maybe using Lightning CSS) but this is a bigger job.

markdalgleish commented 1 year ago

Is this causing an actual user-facing issue for you in terms of browser support? Are there specific browsers you're trying to support that don't support CSS inset? https://caniuse.com/mdn-css_properties_inset

19Qingfeng commented 1 year ago

Is this causing an actual user-facing issue for you in terms of browser support? Are there specific browsers you're trying to support that don't support CSS inset? https://caniuse.com/mdn-css_properties_inset

Yes,I have some user browsers that don't support color abbreviations eg: #0009 not support on chrome 70.At present, my solution is use pnpm patch '@remix-run/dev' and add target in esbuild.

19Qingfeng commented 1 year ago

Is this causing an actual user-facing issue for you in terms of browser support? Are there specific browsers you're trying to support that don't support CSS inset? https://caniuse.com/mdn-css_properties_inset

I had a look into this. It's not strictly related to PostCSS because you can manually write a CSS file containing your example code (body { position: fixed; top:0; bottom:0; left:0; right:0; }) and get the same result.

This is more of an issue with esbuild's CSS minification logic which is quite aggressive by default. I was able to fix it for CSS file imports by setting the targets array on the build inside cssImportsPlugin. I tried setting it to the list of browser versions needed for native JS module support since that's the absolute baseline for Remix:

target: [
  "chrome61",
  "edge16",
  "safari11",
  "ios11",
  "firefox60",
  "opera48",
],

This worked for CSS file imports (i.e. import href from "./styles.css") but this threw errors during our CSS bundle build since that also bundles JS, and esbuild can't handle certain JS features when targeting these browser versions.

I can fix this issue (at least for the default remix template) by raising the browser versions to the following:

target: [
  "chrome63",
  "edge79",
  "safari12",
  "ios12",
  "firefox60",
  "opera50",
],

However, this feels a bit brittle to me since we can't decouple CSS and JS minification options and we might run into further issues with other JS features. I think the safer approach would be to look into a custom CSS minification step in our compiler (maybe using Lightning CSS) but this is a bigger job.

I always think exposing the browserlist is a good op, I saw this discussion, but I don't know what the remix team is choice right now.

github-actions[bot] commented 1 year ago

🤖 Hello there,

We just published version v0.0.0-nightly-3447963-20230819 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

github-actions[bot] commented 1 year ago

🤖 Hello there,

We just published version 2.0.0-pre.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

brophdawg11 commented 1 year ago

Unfortunately we had to revert the fix for this in https://github.com/remix-run/remix/pull/7324 as it caused some other issues with CSS features. Going to re-open this so we can address it via browserslist support early in v2.