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(order): find modules from each chunk in group #455

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ryansully
Copy link

fixes #257

This PR contains a:

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

Motivation / Use-Case

As discussed in #257, when the plugin calls modulesByChunkGroup, it calls the chunk group's getModuleIndex2() function from the ChunkGroup object's _moduleIndices2 Map property. However, the error occurs because the module it's trying to find the index for doesn't exist in _moduleIndices2, causing modulesByChunkGroup to return an empty array. The for loop that uses the array never iterates, so bestMatch is never set and is left undefined, resulting in an error when trying to call .pop() on that undefined object.

This fix checks if getModuleIndex2() returns no matching modules and, if so, dives into each chunk in the chunk group to find matching modules.

Breaking Changes

None.

Additional Info

While this solution dives into a chunk group's chunks to find modules that weren't in the chunk group's _moduleIndices2 property, it might indicate a bug in webpack where that property is not being fully populated as it should be. If that is the case, then this fix should no longer be necessary, but perhaps it wouldn't hurt to have an extra guard for finding chunk modules.

@jsf-clabot
Copy link

jsf-clabot commented Oct 17, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Oct 17, 2019

Codecov Report

Merging #455 (312ad6d) into master (50434b5) will decrease coverage by 0.82%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #455      +/-   ##
==========================================
- Coverage   88.17%   87.34%   -0.83%     
==========================================
  Files           5        5              
  Lines         406      411       +5     
  Branches       86       87       +1     
==========================================
+ Hits          358      359       +1     
- Misses         46       50       +4     
  Partials        2        2              
Impacted Files Coverage Δ
src/index.js 85.57% <33.33%> (-1.68%) ⬇️

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 50434b5...312ad6d. Read the comment docs.

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.

Can we add tests?

@ryansully
Copy link
Author

I will add tests soon so the coverage delta doesn't change.

@ryansully
Copy link
Author

I'm afraid writing a test for this will be difficult; running tests locally fails for me and I haven't been able to get them to pass.

(The lint:prettier script also fails with an error, so I disabled it just to get Jest to run.)

@alexander-akait
Copy link
Member

Just use prettier to fix problem

@doried-a-a
Copy link

Thanks folks for being working on this. Any updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Cannot read property 'pop' of undefined" with a common cache group
5 participants