-
-
Notifications
You must be signed in to change notification settings - Fork 384
fix(index): loading order mismatch #236
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
Conversation
|
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.
Please provide minimum reproducible test repo, also you should add tests to your PR
test rep |
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.
This looks incorrect to me.
Also there no tests so it seem safe to assume it's incorrect.
The repro is far too big for me to try it. Please create a minimal repro. Start from a new empty repo.
@sokra it seems to be that sort func is not correct. |
@ShanaMaid Without minimum reproducible repo and tests, we can't merge this |
I had added test @evilebottnawi |
modules.sort((a, b) => {
let aSort = chunkGroup.getModuleIndex2(a) || 0
let bSort = chunkGroup.getModuleIndex2(b) || 0
if(aSort < bSort){
return 1
}
return -1
}); |
This should be investigated... |
@ShanaMaid I copied your test case and ran it for the fallback code path too. It was broken with your change, but I could fix it by reverting to the original code. This leaves this PR without code change but good testcases. |
@sokra : It happends only when i use this in my webpack config, to get all CSS in one file:
|
Is there no test for the |
@jarandmi let's discuss this in your PR. This PR seem to be done. |
@evilebottnawi This PR is good to be merged, once @ShanaMaid signed the CLA. |
@ShanaMaid what issue it is solved? |
Looks here only tests, merge |
This PR contains a:
Motivation / Use-Case
Breaking Changes
Additional Info