fix: extract css in same order as imports

[wardpeet]

Description

When upgrading rollup to v2 and using @rollup/plugin-node-resolve. My stylesheet wasn't deterministic and tests failed randomly. ModuleIds kept by Rollup are stored by resolve order, not import order. So, it depends on the time it takes to resolve a file.

I added a test where I resolving is done late but the css file still has the correct order.

Fixes #96

I've also made an issue in the rollup repository:
rollup/rollup#3682

[codecov]

Codecov Report

Merging #295 into master will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #295      +/-   ##
==========================================
+ Coverage   92.50%   92.68%   +0.18%     
==========================================
  Files          10       10              
  Lines         320      328       +8     
  Branches      113      115       +2     
==========================================
+ Hits          296      304       +8     
  Misses         23       23              
  Partials        1        1              

Impacted Files	Coverage Δ
src/index.js	99.05% <100.00%> (+0.07%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c38f356...bc94ff7. Read the comment docs.

[egoist]

LGTM, let me know if it's ready to be merged.

[wardpeet]

@egoist I based this PR on #299. If that one is okay, we could merge this one in after the rebase. If not i'll remove the prettier commit.

[BPScott]

Heck yeah I love that the bundle modules stuff seems to work! Fantastic work @wardpeet!

Having two ways of doing this does feel like a maintenance burden though, and might make other features like multiple entry support tougher to implement.

@egoist how would you feel if this only implemented the bundle.modules/ no-treeshake approach - and thus would require rollup >=2.21.0? I'd class that as a breaking change and would require a major version bump but I think it might be worth it for the sake of code simplicity and ease of maintenance.

If that were to happen I think multiple entry support would be pretty easy - it'd be a case of changing this from doing a flatMap over the modules of all bundles to collecting the modules on a per bundle basis. I think something like this (written but not ran):

Object.entries(bundle).forEach(([bundleName, bundle]) => {
  // A list of all modules in this entrypoint  
  const moduleIds = bundle.modules.filter((id) => loaders.isSupported(id));
 
  // Do the stuff that emits out a css file for a given entrypoint
  // Concatenate the found modules, build a sourcemap and run emitFile()
})

Which feels a bit more straight forward compared to the logic for multiple entries that have been attempted in #289 and prior PRs

[BPScott]

Heya @wardpeet, me again. I spotted some awkwardness in the snapshots and as I had an idea what was up I figured I'd make a PR to your PR to demo the fix rather than try and explain it in comments: wardpeet#1

[wardpeet]

@BPScott I do agree that the changes you made remove lots of the $undefined variables that are generated in the newer rollup version. Your changes are technically a breaking change and it would mean it's harder to get reviewed/merged. Terser will remove any of these undefined variables anyway, so there is no perf impact except a more verbose output when not using terser.

If I move back to my first implementation, this would be solved as well. We could land your changes after this one and see if we can bump it as a major version. For now, I want myself unblocked. 😄

[BPScott]

If I move back to my first implementation, this would be solved as well. We could land your changes after this one and see if we can bump it as a major version. For now, I want myself unblocked. 😄

Yep, agree that I'm kinda derailing this PR and getting this behaviour working is top priority :) I'm very happy to say this is good to ship and I'll poke at improvements with potential for breaking changes in a follow-up PR

Thanks again for your fantastic work to get this working!

[wardpeet]

I've run tests on my mac but I couldn't reproduce the failure. Unsure what's happening here. I'll cleanup the PR and hopefully, this can get merged soon after.

[wardpeet]

I converted everything to the first version without any rollup changes needed. This should be good to go.

[wardpeet]

@egoist any chance we could merge this in? :) Happy to adjust the PR with feedback

[github-actions]

🎉 This PR is included in version 3.1.6 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀