-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
feat(npm/react): allow custom webpack config path for react-scripts plugin #16644
feat(npm/react): allow custom webpack config path for react-scripts plugin #16644
Conversation
Thanks for taking the time to open a PR!
|
753d233
to
1b1270b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code seems generally fine. Two comments:
- have you tested this? I see we have not actually added a test. I wonder if we could add a simple unit test, perhaps? I can probably look into this if you aren't sure how or don't have the time. You'd just write it with mocha - two use cases would be loading w/ default location, and loading w/ a custom location.
- if you want this to go out asap, it'll need to target master. Otherwise it will need to wait for the next release (every 2 weeks).
Please let me know
- do you plan to retarget for master, or happy to wait for 2 weeks?
- about adding a unit test - can you do this, or do you need me to look into this?
- have you actually tested this in some capacity, or should I do a manually QA on top of the unit test we should add before merging?
Thanks!
8aefecb
to
1fd4e1c
Compare
@lmiller1990 thanks for quick response
retargeted to
I wasn't able to find any example of those (should I add new npm script?)
I checked it manually and the default behaviour is covered with |
User facing changelog
react-scripts
plugin now could be configured with custom webpack config pathPR Tasks