fix: keep order of script tags #2571
Conversation
🦋 Changeset detectedLatest commit: 4aa5c4e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Feels like it's going in a good direction to start. There are more failures that expected (there are often flakes, but they don't cover the number of failures above) in the Window tests above, can you look into that? |
Most failures are That's not directly related. Not sure how to easily test windows from Linux. But some of them are similarly the result of script tags now in correct order. |
actually. It's the flakes this PR is intended to fix. It seems that rollup-plugin-copy has an error similar to the one I fixed in rollup-plugin-html regarding the ordering |
| @@ -195,6 +195,36 @@ describe('rollup-plugin-html', () => { | |||
| ); | |||
| }); | |||
|
|
|||
| it('can resolve modules in original order', async () => { | |||
There was a problem hiding this comment.
It will resolve modules in the original order, right?
| it('can resolve modules in original order', async () => { | |
| it('resolves modules in original order', async () => { |
|
|
||
| const bundle = await rollup(config); | ||
| const { output } = await bundle.generate(outputConfig); | ||
| expect(output.length).to.equal(4); |
There was a problem hiding this comment.
What is this asserting? What is the actual output?
There was a problem hiding this comment.
Most all of the outputs was reversed. a + b would become b + a.
| expect(appCode).to.include("console.log('entrypoint-a.js');"); | ||
| expect(stripNewlines(getAsset(output, 'index.html').source)).to.equal( | ||
| '<html><head></head><body><h1>Hello world</h1>' + | ||
| '<script type="module" src="./inline-module-5ec680a4efbb48ae254268ab1defe610.js"></script>' + |
There was a problem hiding this comment.
Use the hash variable here again?
There was a problem hiding this comment.
that would be better. I'll change it for the next version.
| @@ -164,7 +164,7 @@ describe('extractModules()', () => { | |||
| attributes: [], | |||
| }, | |||
| ]); | |||
| expect(moduleImports).to.eql([]); | |||
There was a problem hiding this comment.
Can you help me understand this change? From the code changed I don't understand why these tests change.
There was a problem hiding this comment.
See packages/rollup-plugin-html/src/input/getInputData.ts
The order between inline and other modules was lost because they was gathered in two different lists. Then they was merged together upon return, making the inline modules always come after the other modules. I changed it to put all modules including inline modules in the sam list from the start. Thus, the moduleImports lists is no longer empty if there is inlineModules.
| @@ -63,6 +67,7 @@ export function getEntrypointBundles(params: GetEntrypointBundlesParams) { | |||
| } | |||
|
|
|||
| const entrypoints: Entrypoint[] = []; | |||
| const entrypointsUnsorted: EntrypointsUnsorted = {}; | |||
| for (const chunkOrAsset of Object.values(bundle)) { | |||
There was a problem hiding this comment.
So the assets in bundle come to us not in the order that they were parsed in? I wonder why that is. You think it's something in Rollup or in some other Modern Web code?
There was a problem hiding this comment.
My thinking being that if we are creating this asset list incorrectly somewhere else (and not Rollup) we could delve deeper into the issue and try to fix it at it's root rather than sorting the asset/chunk list here.
There was a problem hiding this comment.
The entrypoints is just gathered through bundles and object values. See getEntrypointBundles in the same file. The order could be different from file to file. And on top of that, Object.values is not guaranteed to have a specific order.
| @@ -9,11 +9,11 @@ <h1>Page A</h1> | |||
| </li> | |||
|
|
|||
| <li> | |||
| <a href="/pages/page-B.html">B</a> | |||
| <a href="/pages/page-b.html">B</a> | |||
There was a problem hiding this comment.
I can't see why these changed from upper case to lowercase letters 🤔
There was a problem hiding this comment.
Oh these were changed manually! Do we need to change these? I'd rather make the smallest change possible so it's easier to review. If possible I'd like to revert these changes to the HTML files even though they are probably technically correct.
There was a problem hiding this comment.
Can do this in a separate PR. The demo will not work without the change (on linux) because the file name is page-b.html.
|
Latest checks failed from new bugs in another package, not related to this PR. /home/runner/work/web/web/packages/browser-logs/test/serialize-deserialize.test.ts:1 |
fixes #2466
fixes #2456
What I did
rollup-plugin-html getInputData.ts was adding the inlineModules at the end of the list. I changed it to keep the original order and then used that order in getEntrypointBundles.ts when creating the list of entrypoints.
added another test for the order of script tags
fixed filename case in rollup-plugin-html mpa demo
updated snapshots in rollup-plugin-polyfills-loader tests for the corrected order of the script tags