-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
refactor(server): add updateOptions API helper #2117
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2117 +/- ##
==========================================
+ Coverage 92.53% 92.56% +0.02%
==========================================
Files 32 33 +1
Lines 1260 1265 +5
Branches 362 361 -1
==========================================
+ Hits 1166 1171 +5
Misses 87 87
Partials 7 7
Continue to review full report at Codecov.
|
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.
Good idea, maybe we can move normalize all options inside this util
/cc @hiroppy thoughts? |
Sorry for the late reply.
Agree 👌 |
I agree, but I don't think we should move them in this PR because some of them will cause breaking changes (since the normalizing done in |
We might be better to change the base branch to the |
@hiroppy i think don't need use next branch for this, it is just refactor |
@hiroppy Currently there are no breaking changes, I just moved stuff from the start of the |
/cc @hiroppy for me we can merge this for master version |
beforeAll(() => { | ||
let webpackConfig; | ||
if (data.multiCompiler) { | ||
// eslint-disable-next-line global-require |
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.
You can delete this comment
// eslint-disable-next-line global-require | ||
webpackConfig = require('../../fixtures/multi-compiler-config/webpack.config'); | ||
} else { | ||
// eslint-disable-next-line global-require |
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.
ditto
lib/utils/updateOptions.js
Outdated
no-undefined | ||
*/ | ||
|
||
function updateOptions(compiler, options) { |
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.
@Loonride maybe normalizeOptions
better name here? Anyway code looks good
/cc @hiroppy |
* refactor(server): add update options api helper * test(server): removed global require eslint comments * refactor(server): switch update options to normalize options
For Bugs and Features; did you add new tests?
Yes
Motivation / Use-Case
I am working on going through
createConfig
and moving any CLI-specific behavior to the API. Since there is only onecreateConfig
file, I think it makes sense to parallel it with one API helper to update options and set defaults.I think it will get out of hand to have a helper for each option that needs to have a default setting, such as what I am doing in:
So the above PRs need to be updated to use this if we agree on this solution.
Breaking Changes
None
Additional Info
I will add tests for
transportMode
later, sinceserverMode
andclientMode
will be removed.