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

lib client-api/src/client_api: check type of module.id as fileName #5909

Conversation

ehzhang
Copy link
Contributor

@ehzhang ehzhang commented Mar 6, 2019

Issue: #5907

What I did

Much further downwind when searching through file names, fuzzy-search
and .includes requires that parameters.fileName is a string, and can't handle a
number. This checks to make sure that the fileName is indeed a string,
since webpack may sometimes return the module.id as a number.

https://webpack.js.org/api/module-variables/#moduleid-commonjs

How to test

  • Force a webpack config to mode = "production"
  • Start typing multiple letters
  • See that the search works

@shilman shilman added this to the 5.0.x milestone Mar 6, 2019
@shilman
Copy link
Member

shilman commented Mar 6, 2019

Great bug and thanks for the fix!! @ndelangen @tmeasday seems reasonable?

Edwin Zhang added 2 commits March 6, 2019 09:34
Much further downwind when searching through file names, fuzzy-search
and .includes requires that the fileName parameter is a string, not a
number. This checks to make sure that the fileName is indeed a string,
since webpack may sometimes return the module.id as a number.

https://webpack.js.org/api/module-variables/#moduleid-commonjs
@ehzhang ehzhang force-pushed the ehzhang/check-filename-is-string-before-searching branch from 4c31a1e to bd4beca Compare March 6, 2019 17:40
@ehzhang
Copy link
Contributor Author

ehzhang commented Mar 6, 2019

@shilman i re-pushed to this branch after your approval since i noticed some failing test cases in client_api.test.js, if you'd like to take another pass.

the MockModule in the hot module loading test cases doesn't have an id defined, so it seemed the expected behavior was actually to return undefined.

I changed the little ternary above to return undefined instead of null, and added a test case to make sure we ignore the id if it isn't available from the module

An alternative would be to add fileName: null to the expect calls in the hot module functions, but wasn't sure about y'alls preferences wrt null vs undefined here (I tend to prefer undefined)

@codecov
Copy link

codecov bot commented Mar 6, 2019

Codecov Report

Merging #5909 into next will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##             next    #5909   +/-   ##
=======================================
  Coverage   34.97%   34.97%           
=======================================
  Files         648      648           
  Lines        9480     9480           
  Branches     1333     1360   +27     
=======================================
  Hits         3316     3316           
  Misses       5534     5534           
  Partials      630      630
Impacted Files Coverage Δ
lib/client-api/src/client_api.js 79.41% <100%> (ø) ⬆️

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 2cf21e6...bd4beca. Read the comment docs.

@shilman shilman merged commit 39e28f4 into storybookjs:next Mar 7, 2019
@shilman shilman added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Mar 7, 2019
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Mar 8, 2019
shilman added a commit that referenced this pull request Mar 8, 2019
…g-before-searching

lib client-api/src/client_api: check type of module.id as fileName
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug core patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants