-
-
Notifications
You must be signed in to change notification settings - Fork 597
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: assign cache value for default configs #2013
Conversation
/cc @anshumanv Can you focus on it? I want to do release tomorrow |
|
||
return; | ||
} | ||
}; | ||
|
||
// Given config data, determines the type of config and | ||
// returns final config | ||
const finalize = async (moduleObj, args) => { | ||
const finalize = async (moduleObj, args, isDefault = false) => { |
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.
const finalize = async (moduleObj, args, isDefault = false) => { | |
const finalize = async (moduleObj, args, isDefaultConfig = false) => { |
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.
after ci
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.
@anshumanv Can you fix it, I want to merge it and do release
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.
fixed
@evilebottnawi random failures 😞 |
@anshumanv Let's do rebase |
Codecov Report
@@ Coverage Diff @@
## master #2013 +/- ##
==========================================
+ Coverage 68.96% 69.01% +0.04%
==========================================
Files 85 85
Lines 2436 2443 +7
Branches 489 493 +4
==========================================
+ Hits 1680 1686 +6
- Misses 756 757 +1
Continue to review full report at Codecov.
|
I think we have a bug in implementation, because:
|
@@ -136,6 +136,17 @@ describe('cache related flags from core', () => { | |||
expect(exitCode).toEqual(0); | |||
}); | |||
|
|||
it('should assign cache build dependencies with default config', () => { | |||
// TODO: Fix on windows | |||
if (isWindows) return; |
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.
Avoid this 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.
already skipped for windows, some problems in all tests 😞
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.
Yep, let's focus on it late
This doesn't happen in all tests, which is very strange 😞 |
Can you try it locally? |
passing locally 😞 |
Can you try to use |
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, wait CI green, they are very slow 😞
/cc @webpack/cli-team |
macos is broken today, but should be fine, because linux is green, let's merge and do release |
What kind of change does this PR introduce?
fix
Did you add tests for your changes?
WIP
If relevant, did you update the documentation?
NA
Summary
We assign config path in cache properties passed to the compiler, it didn't take into account the default configs which are resolved so this should fix it.
Does this PR introduce a breaking change?
Nope
Other information
Tests WIP