-
-
Notifications
You must be signed in to change notification settings - Fork 267
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
fix: keep order of script tags #2571
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this asserting? What is the actual output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the hash
variable here again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see why these changed from upper case to lowercase letters 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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