fix: assign cache value for default configs #2013

anshumanv · 2020-11-01T07:37:39Z

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 · 2020-11-02T16:25:12Z

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

snitin315 · 2020-11-03T09:59:32Z

packages/webpack-cli/lib/groups/ConfigGroup.js


        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) => {


Suggested change

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

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

@anshumanv Can you fix it, I want to merge it and do release

anshumanv · 2020-11-04T15:30:03Z

@evilebottnawi random failures 😞

alexander-akait · 2020-11-04T15:32:44Z

@anshumanv Let's do rebase

codecov · 2020-11-04T16:02:16Z

Codecov Report

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

@@            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 · 2020-11-04T16:09:42Z

I think we have a bug in implementation, because:

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

alexander-akait · 2020-11-04T16:10:09Z

test/core-flags/cache-flags.test.js

@@ -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;


Avoid this here

already skipped for windows, some problems in all tests 😞

Yep, let's focus on it late

anshumanv · 2020-11-04T16:13:10Z

I think we have a bug in implementation, because:

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

alexander-akait · 2020-11-04T16:15:08Z

Can you try it locally?

anshumanv · 2020-11-04T16:15:58Z

passing locally 😞

alexander-akait · 2020-11-04T16:18:04Z

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

alexander-akait

Good, wait CI green, they are very slow 😞

anshumanv · 2020-11-04T18:37:25Z

/cc @webpack/cli-team

alexander-akait · 2020-11-04T18:40:51Z

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

webpack-bot added the PR: unreviewed label Nov 1, 2020

anshumanv force-pushed the cache-def branch from 9679a64 to cfcc803 Compare November 3, 2020 07:00

anshumanv marked this pull request as ready for review November 3, 2020 08:53

anshumanv requested a review from a team as a code owner November 3, 2020 08:53

snitin315 reviewed Nov 3, 2020

View reviewed changes

webpack-bot added the PR: conflict label Nov 4, 2020

anshumanv added 4 commits November 4, 2020 21:17

fix: assign cache value for default configs

a232625

fix: assign cache fordefault config

94c4869

tests: fix

a8c5b0e

tests: fix

4755dce

anshumanv force-pushed the cache-def branch from 10e6426 to 4755dce Compare November 4, 2020 15:49

webpack-bot removed the PR: conflict label Nov 4, 2020

alexander-akait reviewed Nov 4, 2020

View reviewed changes

anshumanv added 2 commits November 4, 2020 21:56

fix: tests

9679327

fix: rename

72dede1

anshumanv requested a review from alexander-akait November 4, 2020 18:32

alexander-akait approved these changes Nov 4, 2020

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: unreviewed labels Nov 4, 2020

alexander-akait merged commit d2e3c74 into webpack:master Nov 4, 2020

anshumanv deleted the cache-def branch November 4, 2020 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: assign cache value for default configs #2013

fix: assign cache value for default configs #2013

anshumanv commented Nov 1, 2020

alexander-akait commented Nov 2, 2020

snitin315 Nov 3, 2020

anshumanv Nov 4, 2020

alexander-akait Nov 4, 2020

anshumanv Nov 4, 2020

anshumanv commented Nov 4, 2020

alexander-akait commented Nov 4, 2020

codecov bot commented Nov 4, 2020 •

edited

Loading

alexander-akait commented Nov 4, 2020

alexander-akait Nov 4, 2020

anshumanv Nov 4, 2020

alexander-akait Nov 4, 2020

anshumanv commented Nov 4, 2020

alexander-akait commented Nov 4, 2020

anshumanv commented Nov 4, 2020

alexander-akait commented Nov 4, 2020

alexander-akait left a comment

anshumanv commented Nov 4, 2020

alexander-akait commented Nov 4, 2020

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

fix: assign cache value for default configs #2013

fix: assign cache value for default configs #2013

Conversation

anshumanv commented Nov 1, 2020

alexander-akait commented Nov 2, 2020

snitin315 Nov 3, 2020

Choose a reason for hiding this comment

anshumanv Nov 4, 2020

Choose a reason for hiding this comment

alexander-akait Nov 4, 2020

Choose a reason for hiding this comment

anshumanv Nov 4, 2020

Choose a reason for hiding this comment

anshumanv commented Nov 4, 2020

alexander-akait commented Nov 4, 2020

codecov bot commented Nov 4, 2020 • edited Loading

Codecov Report

alexander-akait commented Nov 4, 2020

alexander-akait Nov 4, 2020

Choose a reason for hiding this comment

anshumanv Nov 4, 2020

Choose a reason for hiding this comment

alexander-akait Nov 4, 2020

Choose a reason for hiding this comment

anshumanv commented Nov 4, 2020

alexander-akait commented Nov 4, 2020

anshumanv commented Nov 4, 2020

alexander-akait commented Nov 4, 2020

alexander-akait left a comment

Choose a reason for hiding this comment

anshumanv commented Nov 4, 2020

alexander-akait commented Nov 4, 2020

codecov bot commented Nov 4, 2020 •

edited

Loading