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 REWRITE] [BIG UPDATE] Change how to process CSS #252

Open
nvh95 opened this issue Aug 23, 2022 · 4 comments
Open

[CSS REWRITE] [BIG UPDATE] Change how to process CSS #252

nvh95 opened this issue Aug 23, 2022 · 4 comments
Labels
feature request Request a new feature

Comments

@nvh95
Copy link
Owner

nvh95 commented Aug 23, 2022

What is it?

Current approach to process CSS

  • Currently, we use Jest Code Transformer to transform CSS. Except Pure CSS, we transform different CSS languages such as Sass, CSS Modules, TailwindCSS, PostCSS... then embed them directly the output CSS to the document.head of jsdom. "Jest Preview server" will serve jsdom as real DOM, so we can see the CSS on the "Jest Preview Dashboard". However, this approach encounters a number of problems
    • (The most annoying one) Jest Code Transformer does not support async actions by default. That means the code must be in "SYNC" style. It's very hard to do some asynchronous actions in a synchronous manner . Sometimes, I could find the sync API to use. Otherwise, I ended up with spawning a new node process, console.log the output the stdout, then reading/ parsing to get the returned value. This is a tricky one and does not scale, and I considered it as a unscale code.
      function havePostCss() {
      // TODO: Since we executing postcssrc() twice, the overall speed is slow
      // We can try to process the PostCSS here to reduce the number of executions
      const result = spawnSync('node', [
      '-e',
      `const postcssrc = require('postcss-load-config');
      postcssrc().then(({ plugins, options }) => {
      console.log(true)
      })
      .catch(error=>{
      if (!/No PostCSS Config found/.test(error.message)) {
      throw new Error("Failed to load PostCSS config", error)
      }
      console.log(false)
      });`,
      ]);
      const stderr = result.stderr.toString('utf-8').trim();
      if (stderr) console.error(stderr);
      if (result.error) throw result.error;
      return result.stdout.toString().trim() === 'true';
      }
    • After PostCSS support #241, the time processing CSS file takes is long. For example, processing a CSS file in a project with PostCSS configuration would take 0.3 seconds on Macbook Air M1. If the user test case contains 10 CSS files, the total time would be 3 seconds. I don't like the idea of waiting 3 seconds before a user's test can run. To be fair, this is not a critical problem, since Jest will cache the computation result, so it's likely you only need to wait once.

Is there any better solution

Yes. But first, let me describe how I come up with that idea.

A few days ago, I was working on #230. The issue is that when a CSS file that contains an @import statement, jest-preview could not load the content of the imported file:

// index.css. This file is imported to the setupTest.js
@import "base.css";
@import "components.css";
@import "utils.css";

If you are curious, I handled this in #248, by inline the imported CSS, using (postcss-import). But the thing is, when opening Jest Preview Dashboard my index.css will automatically request CSS files that are imported. (It's a CSS standard)

Response:

This opens a new horizon for me to resolve all those issues above. What if instead of processing CSS at Jest Code Transformation, the "Jest Preview Dashboard" will process the CSS? I was in the middle of dinner with my family and I had to stop to think and draft the solution.

An HTTP-based approach

This is briefly how it would work:

  • Jest Preview does not process CSS anymore. It just inject the URL to the jsdom's document.head. Like this:
function processCss(src, filename) {
  return {
    code: `const relativeCssPath = "${relativeFilename}";
          const link = document.createElement('link');
          link.rel = 'stylesheet';
          link.href = relativeCssPath;
          document.head.appendChild(link);
          module.exports = JSON.stringify(relativeCssPath);`,
  };
}
  • "Jest Preview Dashboard" will request relativeCssPath from "Jest Preview Server"
  • "Jest Preview Server" will process the CSS and return it to the browser ("Jest Preview Dashboard")
    => User can still see the CSS, the same way it is implemented in Code Transformation.

By doing this, we are moving the computation from Jest process to our "Jest Preview Server" process. Let's see the pros and cons
Pros

  1. Jest will run faster the first time and in the CI environment (see one of my attempts at Add a flag to disable CSS transformation in CI environment #247)
  2. (The most important one) We can write asynchronous code to process CSS
  3. No stale CSS if we update the implementation of the transformer (Jest does not automatically clear cache if code transforms changes. We are clearing the cache in postinstall script, after user installs/ updates jest-preview, see Implement getCacheKey #51 (comment))
  4. As a result of 3, we can remove postinstall script, which is considered bad for "security". See https://socket.dev/npm/package/jest-preview

Cons

  1. jsdom does not contain CSS anymore. If someone wants to do some kinds of "visual testing" using jest-preview/transforms/css. It wouldn't be possible. (Or would it be? Can we reuse processCss?)
  2. Redundant CSS processing. Every time "Jest Preview Dashboard" request a CSS file, "Jest Preview Dashboard" needs to compile the CSS. However, we can handle this by some caching mechanism on both the server (in-memory is enough, cache to disk and LRU are OK but would be overkill) and client (browser cache headers like cache-control/ etag) sides.
  3. (Not really a con) The computation does not go anywhere, it just shifts from Jest to "Jest Preview Server".

Are any caveats?

  • Yes. This supposes to be a BREAKING change. But thanks to the architecture of jest-preview, despite the re-architecture, the user experience remains the same. I have only one concern that Jest Code Transform does not invalidate. But I personally think we can handle it by taking the code transformed itself into account in getCacheKey (Implement getCacheKey #51 (comment)).
  • We might still need to process CSS Modules in Jest Code Transformation because of the cssModulesExportedTokens

Describe alternatives you've considered

Do not implement this at all 😂. Even though the current (Jest Transformation) approach might be slow in CI, but it's not too slow and it's cache-able. If implementing this is too hard. We can just leave it AS-IS

Additional context

#241
#247

@nvh95 nvh95 added the feature request Request a new feature label Aug 23, 2022
@nvh95
Copy link
Owner Author

nvh95 commented Aug 23, 2022

Hey @ntt261298, please see my note above on a new approach to handle CSS. It would be great if you can give some of your thoughts. Thanks.

@ntt261298
Copy link
Collaborator

I think this approach is really promising. (Reduce the test running time, Be able to handle async tasks so we can scale easier in the future,…)
I still have some thoughts about this approach:

  • The time for the browser to fully load the CSS of the preview might be longer than current (But I think reducing the test running time is more important).
  • We might still need to transform CSS module in the transformer. As I understand, we still need to do the expensive task postcssrc() for every test. What if we process the CSS module in the Jest Preview Dashboard? Then it might take a much longer time for the browser to load CSS fully. (Not sure if the user has enough patience to wait for that). I think this is a trade-off.
  • Reducing the time of running postcssrc() might be the problem we need to solve for both approaches (current and new), but it’s probably the really hard one.

To summarize, I think it’s feasible to implement the new approach and will bring more pros than cons.

@nvh95
Copy link
Owner Author

nvh95 commented Sep 8, 2022

[Just a note] This can fix #260

@szamanr
Copy link

szamanr commented Oct 31, 2023

hi, is this on the roadmap? i would like to reduce the runtime in CI or make it only parse css based on some env variable. looks like this rewrite would solve that.

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

No branches or pull requests

3 participants