Skip to content
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(index): loading order mismatch #236

Merged
merged 5 commits into from
Aug 14, 2018

Conversation

ShanaMaid
Copy link
Contributor

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Breaking Changes

Additional Info

@jsf-clabot
Copy link

jsf-clabot commented Aug 8, 2018

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ sokra
❌ ShanaMaid

Copy link
Member

@alexander-akait alexander-akait left a 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

@ShanaMaid
Copy link
Contributor Author

test rep
#188 (comment)

Copy link
Member

@sokra sokra left a 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.

@michael-ciniawsky michael-ciniawsky changed the title fix css files load order mismatch #188 fix(index): loading order mismatch Aug 8, 2018
@ShanaMaid
Copy link
Contributor Author

@sokra
this mini rep offered by @boraturant , difference is that I use local mini-css-extract-plugin builded by my pr #236.
comparing with mini-css-extract-plugin@0.4.1, file change is 856da5e

it seems to be that sort func is not correct.

@alexander-akait
Copy link
Member

@ShanaMaid Without minimum reproducible repo and tests, we can't merge this

@ShanaMaid
Copy link
Contributor Author

ShanaMaid commented Aug 9, 2018

I had added test @evilebottnawi
image

@jarandmi
Copy link

jarandmi commented Aug 13, 2018

chunkGroup.getModuleIndex2() is often returned as undefined (i'm not sure why). This should also be handled in the if (typeof chunkGroup.getModuleIndex2 === 'function') block. If undefined, it should be sorted to the bottom of the array.

modules.sort((a, b) => { 
        let aSort = chunkGroup.getModuleIndex2(a) || 0
        let bSort = chunkGroup.getModuleIndex2(b) || 0

        if(aSort < bSort){
          return 1
        }
        return -1
});

@sokra
Copy link
Member

sokra commented Aug 13, 2018

chunkGroup.getModuleIndex2() is often returned as undefined (i'm not sure why)

This should be investigated...

@sokra
Copy link
Member

sokra commented Aug 13, 2018

@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.

@jarandmi
Copy link

jarandmi commented Aug 13, 2018

@sokra : It happends only when i use this in my webpack config, to get all CSS in one file:

            splitChunks: {
                cacheGroups: {
                    styles: {
                        name: 'style',
                        test: module =>
                            module.nameForCondition &&
                            /\.(scss|css)$/.test(module.nameForCondition()),
                        chunks: 'all',
                    },
                },
            },
        }

@jarandmi
Copy link

jarandmi commented Aug 13, 2018

Is there no test for the Extracting all CSS in a single file case?

@sokra
Copy link
Member

sokra commented Aug 14, 2018

@jarandmi let's discuss this in your PR.

This PR seem to be done.

@sokra
Copy link
Member

sokra commented Aug 14, 2018

@evilebottnawi This PR is good to be merged, once @ShanaMaid signed the CLA.

@alexander-akait
Copy link
Member

@ShanaMaid what issue it is solved?

@alexander-akait
Copy link
Member

Looks here only tests, merge

@alexander-akait alexander-akait merged commit 97e2f4c into webpack-contrib:master Aug 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants