-
-
Notifications
You must be signed in to change notification settings - Fork 620
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: 🐛 do not apply own defaults while setting mode #1565
Conversation
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.
Can we add tests?
Adding in a while |
@evilebottnawi added 👍 |
@@ -40,6 +41,8 @@ describe('GroupHelper', function () { | |||
]); | |||
|
|||
const result = group.run(); | |||
// ensure no other properties are added | |||
expect(result.options).toMatchObject({ mode: 'development' }); |
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.
Can we look on plugins here?
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.
Old method is only adding config options from the file and no plugins
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.
We should look what we don't have two terser plugins here
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.
Can you suggest ways to check it here? We're just passing mode to the compiler, so since core doesn't have two, we won't have two either.
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.
Hm, maybe we can add a test there manually added Terser
to minimizer with custom options, for commentsFilename, not the best solution, but will be work
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.
Added, something wrong with CI, will take a look tomm
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.
@evilebottnawi seems good now, PTAL
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.
/cc @webpack/cli-team
Need look at CI, idk what's failing |
@evilebottnawi fixed now, had to do some changes to the install script since it was running into race condition. Looks all good now. |
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 job!
What kind of change does this PR introduce?
bugfix
Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
NA
Summary
Does this PR introduce a breaking change?
No
Other information