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

hot.accept tap interceptor returns T/F based on the number of args #6962

Merged
merged 1 commit into from
Apr 17, 2018

Conversation

justinhelmer
Copy link

@justinhelmer justinhelmer commented Apr 4, 2018

addresses #6919

What kind of change does this PR introduce?

bugfix

Did you add tests for your changes?

yes

If relevant, link to documentation update:

N/A

Summary

#6919

Does this PR introduce a breaking change?

Not to any external interface

@jsf-clabot
Copy link

jsf-clabot commented Apr 4, 2018

CLA assistant check
All committers have signed the CLA.

@ooflorent
Copy link
Member

For the test case, create a folder named issue-6962 inside test/hotCases/ and:

  • create test/hotCases/issue-6962/webpack.config.js with the buggy config
  • add your files to assert the DefinePlugin worked and the HMR is still working

You can run the hot test suite using:

yarn test:integration --grep Hot

@ooflorent
Copy link
Member

If you returns true for the hook it means that the parser must stop the traversing so the arguments are not parsed.

For single argument module.hot.accept you must return true because no further processing is required. However, if there are more arguments, you should return false or continue the parsing then return true:

parser.walkExpression(expr.arguments[1]); // other args are ignored
return true;

Copy link
Member

@ooflorent ooflorent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comments

@webpack-bot
Copy link
Contributor

@justinhelmer Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@ooflorent Please review the new changes.

@justinhelmer
Copy link
Author

@ooflorent - sorry for the delay on this. I was following the original issue thread and missed your comments.

One thing to note for others reading this thread - tests must be in a subdirectory of hotCases - i.e.:

test/hotCases/define/issue-6962/webpack.config.js

as opposed to:

test/hotCases/issue-6962/webpack.config.js

Then, after familiarizing myself with the existing test patterns and your comments, I was able to get it working. I have pushed the changes for final review.


Now the question remains - in order to resolve #6919 - I need to make a similar change to the v3 branch. I have done so here: bc840ec...justinhelmer:bug/6919-v3. It seems Tapable isn't used and I'm guessing the testing pattern is different as well. Any help will be appreciated.

@justinhelmer
Copy link
Author

I squashed into the original commit which is why I cannot resolve the requested change.

@webpack-bot
Copy link
Contributor

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@sokra sokra merged commit aee2491 into webpack:master Apr 17, 2018
@sokra
Copy link
Member

sokra commented Apr 17, 2018

Thanks

@justinhelmer
Copy link
Author

@sokra @ooflorent - what about wp3? (per #6919)

@ooflorent
Copy link
Member

Could you submit a PR for the v3 branch?

@justinhelmer
Copy link
Author

@ooflorent - I would love to, but am currently blocked. Awaiting feedback from someone who is knowledgeable of the system. Please see my most recent two comments on #6919

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

5 participants