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(config): support pass false to plugins #4949

Merged
merged 10 commits into from Dec 12, 2023

Conversation

puppet-666
Copy link
Contributor

@puppet-666 puppet-666 commented Dec 8, 2023

Summary

To support pass false to plugins

resolve #4939
resolve #4926

Test Plan

This is my first time contributing to rspack. At present, I only know how to create tests in the 'packages \ rspack \ tests \ configCases' directory, but I am not very familiar with the running logic of test cases. Can you give me some help?

Require Documentation?

  • [ x ] No
  • Yes, the corresponding rspack-website PR is __

@puppet-666 puppet-666 requested review from a team, LingyuCoder and h-a-n-a December 8, 2023 03:16
@CLAassistant
Copy link

CLAassistant commented Dec 8, 2023

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the release: bug fix release: bug related release(mr only) label Dec 8, 2023
ScriptedAlchemy
ScriptedAlchemy previously approved these changes Dec 8, 2023
@hardfist
Copy link
Contributor

hardfist commented Dec 8, 2023

@puppet-666 please sign the cla

@puppet-666
Copy link
Contributor Author

@puppet-666 please sign the cla

done

@hardfist
Copy link
Contributor

hardfist commented Dec 8, 2023

https://web-infra-dev.github.io/rspack-dev-guide/testing/intro.html @puppet-666 you can read this about how rspack test

@puppet-666
Copy link
Contributor Author

web-infra-dev.github.io/rspack-dev-guide/testing/intro.html @puppet-666 you can read this about how rspack test

Thanks for your answer. Are test cases required for this merge? If so I will add test cases

@hardfist
Copy link
Contributor

hardfist commented Dec 8, 2023

@puppet-666 I think we should align this with webpack and allow all falsy value here, you can take webpack/webpack#17339 for reference(no need to support falsy value for loader in this PR)

@puppet-666
Copy link
Contributor Author

@puppet-666 I think we should align this with webpack and allow all falsy value here, you can take webpack/webpack#17339 for reference(no need to support falsy value for loader in this PR)

@hardfist
Thank you for your advice. There are two places where the new mr may need your opinion

  • I've looked at the Plugin Hook Support List and found that the make hook is complete, but I'm not sure if I need to add a Child Compilation test or not
  • I'm not very good at using zod, so I may need a review from your side

hardfist
hardfist previously approved these changes Dec 12, 2023
@hardfist
Copy link
Contributor

thanks!

@LingyuCoder LingyuCoder merged commit 6af7853 into web-infra-dev:main Dec 12, 2023
16 checks passed
ahabhgk pushed a commit that referenced this pull request Jan 2, 2024
* fix(config): support pass false to plugins

* fix: prettier format

* test(config): support pass false to plugins

* fix(test): support pass false to plugins

* fix(config): support falsy

* fix: zod falsy type

* fix: zod falsy string init value

* fix: Temporarily remove support for falsy in rules

* fix: minimizer apply

* fix: falsy test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: bug fix release: bug related release(mr only)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support pass false to plugins Unable to resolve variables defined in definePlugin in rspack mode
6 participants