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: assign cache value for default configs #2013

Merged
merged 6 commits into from Nov 4, 2020

Conversation

anshumanv
Copy link
Member

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

@alexander-akait
Copy link
Member

/cc @anshumanv Can you focus on it? I want to do release tomorrow

@anshumanv anshumanv marked this pull request as ready for review November 3, 2020 08:53
@anshumanv anshumanv requested a review from a team as a code owner November 3, 2020 08:53

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) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const finalize = async (moduleObj, args, isDefault = false) => {
const finalize = async (moduleObj, args, isDefaultConfig = false) => {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after ci

Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@anshumanv
Copy link
Member Author

@evilebottnawi random failures 😞

@alexander-akait
Copy link
Member

@anshumanv Let's do rebase

@codecov
Copy link

codecov bot commented Nov 4, 2020

Codecov Report

Merging #2013 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
packages/webpack-cli/lib/groups/resolveConfig.js 98.23% <100.00%> (+0.03%) ⬆️
packages/webpack-cli/lib/utils/flag-defaults.js 100.00% <100.00%> (ø)
packages/webpack-cli/lib/webpack-cli.js 95.62% <100.00%> (ø)
packages/serve/src/parseArgs.ts 87.50% <0.00%> (-2.98%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c5a2ba...72dede1. Read the comment docs.

@alexander-akait
Copy link
Member

I think we have a bug in implementation, because:

  buildDependencies: {
        defaultWebpack: [
          '/home/runner/work/webpack-cli/webpack-cli/node_modules/webpack/lib/'
        ]
      },

@@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid this here

Copy link
Member Author

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 😞

Copy link
Member

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

@anshumanv
Copy link
Member Author

I think we have a bug in implementation, because:

This doesn't happen in all tests, which is very strange 😞

@alexander-akait
Copy link
Member

Can you try it locally?

@anshumanv
Copy link
Member Author

passing locally 😞

@alexander-akait
Copy link
Member

Can you try to use node_modules/bin/jest test/path/to/name.test.js for this test (change it in github actions file), so we will run the only related tests

Copy link
Member

@alexander-akait alexander-akait left a 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 😞

@anshumanv
Copy link
Member Author

/cc @webpack/cli-team

@alexander-akait
Copy link
Member

macos is broken today, but should be fine, because linux is green, let's merge and do release

@alexander-akait alexander-akait merged commit d2e3c74 into webpack:master Nov 4, 2020
@anshumanv anshumanv deleted the cache-def branch November 4, 2020 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants