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

Avoid writing to output files when no changes #6550

Merged

Conversation

g3offrey
Copy link
Contributor

@g3offrey g3offrey commented Dec 15, 2021

Hello,

First of all thank you for creating Tailwind. I am a big fan of this project 😄
I am also a fan of the Remix project.

I wanted to use Remix in addition of Tailwind, but I encountered an issue.
Indeed the Tailwind CLI writes the result of the CSS processing to the configured output file even if the file already contains this same output (if we change TSX for example). This cause a double rebuild Remix side (one useless for the unmodified CSS file and one for the TSX change).
I simply did a comparison before writing to the output file and this fix the issue.

However I think I can still improve this PR, as the Tailwind CLI still output "Rebuilding ..." even if it is not really writing any output.
What do you think ? 😅

If you want to try this easily I created a reproduction repository here. Simply modify "Change me" in app/routes/index.tsx and you'll see that Tailwind rewrite the css file.

Thank you per advance.

@g3offrey g3offrey changed the title fix(cli): avoid write same output when no changes Avoid writing to output files when no changes Dec 15, 2021
@kentcdodds
Copy link

I'm case it's helpful, here's the remix issue about this: remix-run/remix#714

And here's how I solved this for postcss-cli: postcss/postcss-cli#417

@RobinMalfait RobinMalfait self-assigned this Jan 4, 2022
This function will check a cache, it will only write the file if:
- The modified timestamps changed since last time we wrote something.
  This is useful to know if something changed by another tool or
  manually without diffing the full file.
- The contents changed.
@RobinMalfait
Copy link
Member

Hey! Thank you for your PR!
Much appreciated! 🙏

I think this PR is looking good, I was a bit afraid about that single variable to keep track, but since this is the CLI this is fine. In a non-CLI environment we can't just keep track of this because then we would need to take multiple processes into account and update the internal context and yada yada yada.

The downside is that if you change the CSS file while the watcher is running, then we don't "recover" from this change. In that case the solution that @kentcdodds provided to the postcss-cli is better. However, I don't think we want the overhead of reading the target css every time we make a change.

Regarding the Rebuilding... message, we are technically still rebuilding so there is still time spent doing that. I think it is fine to keep that as is.


I'll make a few small changes that will recover from changes by an external project while the CLI is watching, without reading the full file and comparing it.

@kentcdodds
Copy link

Notice in my testing that reading even many very large files won't be a performance problem at all (we're talking less than a few milliseconds). I think that's the best solution with basically no downside.

@RobinMalfait
Copy link
Member

It's fast, and honestly probably neglectable, but apart from reading the file, you also have to compare the files.

❯ hyperfine 'node a.js' 'node b.js' --warmup 15
Benchmark 1: node a.js
  Time (mean ± σ):      38.5 ms ±   1.3 ms    [User: 30.1 ms, System: 8.1 ms]
  Range (min … max):    37.0 ms …  45.3 ms    71 runs

Benchmark 2: node b.js
  Time (mean ± σ):      25.6 ms ±   1.1 ms    [User: 20.6 ms, System: 3.7 ms]
  Range (min … max):    24.4 ms …  30.3 ms    106 runs

Summary
  'node b.js' ran
    1.50 ± 0.08 times faster than 'node a.js'

Where a.js is defined as:

let fs = require("fs");

async function compare() {
  let [a, b] = await Promise.all([
    fs.promises.readFile("./file.a.css", "utf8"),
    fs.promises.readFile("./file.b.css", "utf8"),
  ]);

  return a === b;
}

async function run() {
  for (let _ of Array(100)) {
    await compare();
  }
}

run();

And b.js is defined as:

let fs = require("fs");

async function compare() {
  let [a, b] = await Promise.all([
    fs.promises.stat("./file.a.css", "utf8"),
    fs.promises.stat("./file.b.css", "utf8"),
  ]);

  return a.mtimeMs === b.mtimeMs;
}

async function run() {
  for (let _ of Array(100)) {
    await compare();
  }
}

run();

Both file.a.css and file.b.css are the same file, and both are 92K in file size. Generated using the following tailwind.config.js file:

module.exports = {
  content: [],
  safelist: [{
    pattern: /bg-/g,
    variants: ['hover', 'focus']
  }],
  theme: {
    extend: {},
  },
  plugins: [],
}

@kentcdodds
Copy link

Those results look pretty negligible to me 🤷‍♂️

Turns out that reading files and comparing them is fairly fast and there
is no huge benefit over only using the Stats of the file and keeping
track of that information.

Thanks @kentcdodds!
@RobinMalfait RobinMalfait merged commit 0bcd628 into tailwindlabs:master Jan 4, 2022
@kentcdodds
Copy link

Super! Thank you for working on this!

@g3offrey
Copy link
Contributor Author

g3offrey commented Jan 4, 2022

Thanks a lot @RobinMalfait and @kentcdodds for iterating on my pull request and explaining the proposals 😁
The merged solution is neat 👌

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

Successfully merging this pull request may close these issues.

3 participants