The latest updates on your projects. Learn more about Vercel for Git ↗︎

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.39%. Comparing base (184bc4e) to head (ddf4b13).
Report is 1 commits behind head on master.

@@           Coverage Diff           @@
##           master    #5644   +/-   ##
=======================================
  Coverage   99.39%   99.39%           
=======================================
  Files         242      242           
  Lines        9348     9349    +1     
  Branches     2470     2470           
=======================================
+ Hits         9291     9292    +1     
  Misses         48       48           
  Partials        9        9           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Makes sense, though I guess it is probably hard to write a meaningful test. One could write a manual test that bundles the same input twice and introduces a renderChunk hook that slows down the rendering of one or the other chunk in both runs and in the end verifies that the hashes for both runs match.
I think that would fit into misc.js where we already have a test that chunks are sorted.

I wonder, though, if we really need to manually sort the placeholders before iterating. There is a placeholders Set in transformChunksAndGenerateContentHashes that is created synchronously from the chunks respecting their order. I wonder if that Set would have a sufficiently stable insertion order so that when returned from transformChunksAndGenerateContentHashes and used in generateFinalHashes would have the same effect as the manual sorting?

One could write a manual test that bundles the same input twice and introduces a renderChunk hook that slows down the rendering of one or the other chunk in both runs and in the end verifies that the hashes for both runs match.
I think that would fit into misc.js where we already have a test that chunks are sorted.

I can take a look into drafting that up. Just to insure I found what you're referring to, you're suggesting modeling that test off of this one?

I wonder, though, if we really need to manually sort the placeholders before iterating. There is a placeholders Set in transformChunksAndGenerateContentHashes that is created synchronously from the chunks respecting their order. I wonder if that Set would have a sufficiently stable insertion order so that when returned from transformChunksAndGenerateContentHashes and used in generateFinalHashes would have the same effect as the manual sorting?

Just to be clear, you're suggesting the following?

1. Have transformChunksAndGenerateContentHashes return the placeholders set, which contains all the placeholders all the hashPlaceholders subsequently used as keys for renderedChunksByPlaceholder
2. Pass the resulting placeholders set as an additional param to generateFinalHashes
3. Iterate over the placeholder set and retrieve chunks from renderedChunksByPlaceholder by those placeholder keys

I think that could work too. As you said, it seems that the placeholder set is synchronously built and should be deterministic. This would also avoid the additional overhead of the sort by keys, which is likely fairly minimal, but can empathize with the desire to squeeze performance and memory overhead. I think my only concern with using placeholders is that there is an implicit assumption that the the placeholders set and renderedChunksByPlaceholder keys have the same one-to-one correspondence (i.e. same size and values). That does appear to be the case, but if that assumption no longer holds true, then there might be a bug. Sorting the keys of renderedChunksByPlaceholder avoids that potential problem, but does have more cost.

I can refactor this change with your suggested approach and vet it against my use case if you think that is preferable. I'm good either way.

you're suggesting modeling that test off of this one?

Yes, roughly, except that of course you would build twice. And note that this is an old test, you can use async-await these days to make the test much nicer ;)

I can refactor this change with your suggested approach and vet it against my use case if you think that is preferable. I'm good either way.

That is why I would also like to have this test. As I see it, the renderChunk hook is the major source of asynchronicity we have, so if we have a test specifically for that, then I would feel good that we do not break it in the future.
I must admit that it is micro-optimization to use placeholders instead of sorting, on the other hand, the data structure is already created. And my expectation is that is is as deterministic as sorting by placeholder name. Which is to say, the latter could also be broken. Which brings me back to the test ;)

In any case, thank you so much for looking into this and providing a solution for this issue in the first place! I know that indeterminism is a major source of pain for those affected by it, and you are making it better for everyone. If you need help, let me know, then I can also have a look at writing that test.

you're suggesting modeling that test off of this one?

Yes, roughly, except that of course you would build twice. And note that this is an old test, you can use async-await these days to make the test much nicer ;)

I can refactor this change with your suggested approach and vet it against my use case if you think that is preferable. I'm good either way.

That is why I would also like to have this test. As I see it, the renderChunk hook is the major source of asynchronicity we have, so if we have a test specifically for that, then I would feel good that we do not break it in the future. I must admit that it is micro-optimization to use placeholders instead of sorting, on the other hand, the data structure is already created. And my expectation is that is is as deterministic as sorting by placeholder name. Which is to say, the latter could also be broken. Which brings me back to the test ;)

In any case, thank you so much for looking into this and providing a solution for this issue in the first place! I know that indeterminism is a major source of pain for those affected by it, and you are making it better for everyone. If you need help, let me know, then I can also have a look at writing that test.

Thanks for your thoughts and guidance! I'll take a swing at the test shortly and also adjust the PR to use the existing placeholder set. I'll report back once that is done or if I could use any assistance with the test. Thanks again!

@lukastaegert I removed the sorting, switched to the stable placeholder set, and introduced a test.

I validated that the test fails on master as well.

Test failing on master

I've updated the PR description to reflect the latest changes. Let me know if you'd like me to make any additional changes.

This PR has been released as part of rollup@4.22.0. You can test it via npm install rollup.