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

CSS bundling interop #1

Closed
romainmenke opened this issue Jul 29, 2023 · 8 comments
Closed

CSS bundling interop #1

romainmenke opened this issue Jul 29, 2023 · 8 comments

Comments

@romainmenke
Copy link
Owner

romainmenke commented Jul 29, 2023

Hi @evanw @devongovett,

While working on some fixes for postcss-import I wanted to compare the behavior of that bundler with how browsers natively handle @import.

I also wanted to compare with other CSS bundlers (both esbuild and lightningcss can bundle CSS).

Not to compete with either but because I value interop.
I strongly believe that multiple tools that bundle CSS should do so in similar and predictable ways.

Users should not have to refactor their source code when switching between PostCSS, esbuild, lightningcss, ...


This repository contains a bunch of tests that all pass in either Firefox or Chrome.
(Firefox crashes on some cyclical imports, Chrome doesn't support supports() conditions in imports.)

Since I personally do not use esbuild or lightningcss I am not going to invest time to file bugs for each test failure.

I do however think it would be valuable to have a shared test suite.
Think "WPT" but for CSS bundlers.

A shared set of tests that multiple parties agree on and that each can use to verify their own project.

The intention is not to enforce a passing grade or to blame anyone in case of a test failure.

Each tool is obviously free to choose a "non-standard" direction if that makes more sense to them.

Main benefits from my point of view :

  • each maintainer benefits from increased test coverage
  • CSS authors have increased interop between tools
  • easier for new tools to be created that are compatible with existing tools

Is this something you want to collaborate on?

@devongovett
Copy link

Thanks for making these tests! Looks valuable. I'll take a more detailed look soon.

@evanw
Copy link
Contributor

evanw commented Jul 31, 2023

Thanks for putting these together. Test cases are a great way to promote interop.

I took a brief look at these. It looks like you're primarily asking for esbuild to implement bundling of conditional imports. This is not a bug with esbuild. It's just not feature that esbuild currently implements, which should already be indicated by the error message that esbuild generates when you attempt to do this. See: evanw/esbuild#953.

I'm also seeing errors of the form No loader is configured for ".png" files. This is not a problem with esbuild even though your tests make it appear to be so. Instead it is a configuration error. You need to tell esbuild what you want it to do with .png files (e.g. --loader:.png=copy would copy them alongside the bundle, --loader:.png=dataurl would inline them in the CSS, and --external:*.png would pass the paths through without bundling them).

Regarding the test suite: I could not get it to work. I don't use puppeteer myself, and I'm not going to look into fixing this. Running the suggested command didn't work (note that I'm running this in GitHub Codespaces). The error looks like this:

file:///workspaces/css-import-tests/node_modules/puppeteer-core/lib/esm/puppeteer/node/ProductLauncher.js:283
                    throw new Error(`Could not find Firefox (rev. ${this.puppeteer.browserRevision}). This can occur if either\n` +
                          ^

Error: Could not find Firefox (rev. latest). This can occur if either
 1. you did not perform an installation for Firefox before running the script (e.g. `PUPPETEER_PRODUCT=firefox npm install`) or
 2. your cache path is incorrectly configured (which is: /home/codespace/.cache/puppeteer).
For (2), check out our guide on configuring puppeteer at https://pptr.dev/guides/configuration.

I was able to run your tests by commenting out the Firefox stuff and renaming postcssImport to postcssImportDev.

Edit: For my own reference, the additional commands required to get this to run are:

npm run install:with-firefox
sudo apt-get update
sudo apt-get install -y libxtst6
sudo apt-get install -y libdbus-glib-1-dev
sed -i 's/postcssImportDev(/postcssImport(/' util/test.mjs

After a while the script prints out a markdown table and hangs. Then you have to manually exit the script.

@romainmenke
Copy link
Owner Author

I'm also seeing errors of the form No loader is configured for ".png" files. This is not a problem with esbuild even though your tests make it appear to be so. Instead it is a configuration error. You need to tell esbuild what you want it to do with .png files (e.g. --loader:.png=copy would copy them alongside the bundle, --loader:.png=dataurl would inline them in the CSS, and --external:*.png would pass the paths through without bundling them).

Thank you for that! I've changed the settings and many of those tests now pass.


I took a brief look at these. It looks like you're primarily asking for esbuild to implement bundling of conditional imports. This is not a bug with esbuild.

I am not asking for any fixes or feature additions here :)
I think it's fine if any tool only supports a specific subset of features.

However I think it's valuable to have a shared test suite to verify that supported features work consistently in multiple tools.

postcss-import for example doesn't have the correct source order when the same stylesheet is imported twice. This makes it complicated to drop postcss-import and adopt esbuild or lightningcss (or the other way around).


I don't think it is important to compare these tools publicly.
I don't intend to add a table to something like postcss-import (or anything else) to compare it with other bundlers.

It is easy to create a negative spiral and I am not interested in that.


Regarding the test suite: I could not get it to work. I don't use puppeteer myself, and I'm not going to look into fixing this.

Puppeteer makes it possible but not easy to setup and test against Firefox.
Firefox is currently the only browser that supports @import "foo.css" supports(...)

The setup command here is npm run install:with-firefox : https://github.com/romainmenke/css-import-tests/blob/main/package.json#L8


The current test suite is something that worked for me when trying to fix bugs in postcss-import.

If there is interest to share a test suite between multiple tools then I think we first need to align on what that would look like.

  • a (npm?) package that everyone imports in their project
  • a standalone test suite that pulls in the latest version (or latest commit on main) of each project
  • something that does static analysis?
  • something that does a runtime check (like it does now)
  • ...

Our projects all have different setups, constraints, ...

@romainmenke
Copy link
Owner Author

After a while the script prints out a markdown table and hangs. Then you have to manually exit the script.

Correct, postcss-import as published on npm is very much broken, working on it.

@evanw
Copy link
Contributor

evanw commented Aug 7, 2023

I reformatted these tests as a single HTML page to make them easy to run: https://github.com/evanw/css-import-tests. You can now just open the page in a web browser to run the tests (and in theory just swap the CSS entry point file with a bundled one to test the output from a specific bundler). Some additional things I noticed:

  • Importing with supports() only works in Firefox. It does not work in Chrome or Safari.
  • The last three cycle tests only works in Chrome and Safari. They do not work in Firefox (and will even crash the tab when developer tools is opened).
  • The test subresource/003 references a file that doesn't exist. Specifically subresource/003/styles/green.css references ../../green.png which would be subresource/green.png but the file lives at subresource/003/green.png instead. I'm assuming that this test is checking for the behavior of web servers where a top-level ../ path segment is a no-op. This is very subtle and seems like something that would be undesirable for a bundler to support since it would mask bugs and/or be very confusing.

After looking at these in detail, I'm not sure what to think about the tests that require the files to be served by a web server at a certain location. Specifically:

  • subresource/003 (requires one of the ../ path segments to be a no-op)
  • subresource/006 (has a path with a leading / that is a web server root, not a file system root)
  • 001/absolute-url, at-media/001/absolute-url, at-media/009, at-media/010, at-media/011, url-format/001/absolute-url, url-format/002/absolute-url, url-format/003/absolute-url (contains paths with the http:// protocol)

I'm not sure it makes sense for a CSS bundler to support these cases. At least, I'm not planning on supporting these with esbuild. It would be convenient for me to have the test suite for CSS features be separate from the test suite that tests browser emulation. That way you could reasonably expect a CSS bundler to eventually pass all of the CSS feature tests, and the browser emulation tests could be informational only without any expectation of CSS bundlers actually passing them.

Edit: I added another HTML page to that repo with the subset of tests that I would expect a CSS bundler to pass.

@romainmenke
Copy link
Owner Author

Thank you @evanw for taking an in-depth look.

I think failures fall in roughly these categories :

  1. the feature is a core feature and is incorrectly implemented by the bundler
  2. the feature is unsupported by the bundler at this time
  3. making this test pass would make some other test fail, only a real browser can pass both
  4. only a real browser can pass this test, a bundler can not mimic this behavior

I think there is value in charting all cases.
But it would be good to group them so that implementers can pick which test groups they want to pass.

Tests under 1. are the most straight forward, it's only a matter of time/funding/interest to get these resolved.

tests must not have :

  • cascade layers
  • conditions
  • data urls
  • protocols/domains in urls
  • ...

2. Is already more debatable. Is it really valuable to have support supports() conditions in a bundler? It think it's a valid choice for any tool to keep @import support more basic. But if there is support then it should be correctly implemented.

Tests should be grouped so that implementers can pick the test suites that are relevant for them.

3. Is the tricky group.
A good example is a mix of relative import paths and some with a http:// protocol:

  • inline everything and lose the "liveness" of the remote resource?
  • inline relative paths after the remote resource imports?
  • only inline everything after the last remote resource import?
  • just error and refuse to do anything?

For 99% of cases the second option will just work.
The remote resource is most likely an external stylesheet with some @font-face rules.
Re-ordering won't change anything.

This is what postcss-import does and it works for most users, but it's still wrong.

I think it makes sense to align on these cases between tools.
We choose which tests we fail and we expect all tools to fail these tests specifically. Passing one of those should be a bad thing.

4. Is only relevant for completeness. To record which features can not be implemented in a bundler.

Maybe someone finds something clever to implement one of these after all.
Not having tests for these things just creates a blind spot.

The nuance between 3. and 4. is that 4. really is impossible and 3. can achieved by sacrificing correctness in some other area.


This would roughly look like this :

- core features
- sub features
  - cascade layers
  - media conditions
  - supports conditions
  - data url
  - ...
- must fail
- unimplementable

Interactions between multiple sub features are problematic.
Testing interactions between cascade layers and supports conditions is only possible when both are implemented.

Is this a separate group? Or is there a pre-defined order in which features are assumed to be implemented. e.g. if you support supports conditions then you also support media conditions, so interactions can be tested in the supports conditions group.


I don't really want to discuss individual cases in this thread, because it's easy to get side tracked.

But happy to discuss any of them in separate issues :)


The last three cycle tests only works in Chrome and Safari. They do not work in Firefox (and will even crash the tab when developer tools is opened).

Yes, I filed a bug report : https://bugzilla.mozilla.org/show_bug.cgi?id=1844683

@evanw
Copy link
Contributor

evanw commented Aug 8, 2023

The latest version of esbuild (v0.18.20) implements conditional imports. I believe this means esbuild should now pass all tests regarding CSS features except for at-layer/003. You should see esbuild pass more tests if you update esbuild and rerun your tests. Thanks again for making these tests. They were very helpful when implementing these features.

The test at-layer/003 is great because it made me realize that I'm not actually sure how CSS bundling is supposed to work. I've been treating @import duplicates as a "last one wins" thing but @layer is a "first one wins" thing. And I can't just always inline every @import because that causes infinite expansion due to cycles. So thanks for including that test. I'm going to ask around to see if I can figure out how I'm supposed to bundle @import + @layer + cycles. In the meantime esbuild will bundle cases like this incorrectly.

@romainmenke
Copy link
Owner Author

That is good to hear :)

Nice that these tests are useful.


Do you have any feedback or insights on how we can make these tests useful going forward?

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

3 participants