Feature/find config recursively #693

lakatostamas · 2018-11-18T17:11:05Z

What kind of change does this PR introduce?
Feature.
Did you add tests for your changes?
Yes.
If relevant, did you update the documentation?
Should I open a PR in this repo: https://github.com/webpack/webpack.js.org ?
Summary
Issue: #689
I used a third-party lib to find the config file recursively. This lib exactly does what we need, but if you think we shouldn't use a third-party for this I remove it and implement it by myself. I checked the "old" solution in config-loader and it used cosmiconfig but I think findup-sync is more suitable for us in this repository. (https://github.com/webpack-contrib/config-loader/blob/859799105c17d89a481b3849f75497ed4f656348/lib/load.js#L63)

Does this PR introduce a breaking change?
No (I guess)

Other information
Closes #689

jsf-clabot · 2018-11-18T17:11:06Z

All committers have signed the CLA.

hemal7735 · 2018-11-19T13:18:12Z

@lakatostamas can you also add some negative test-cases to check what happens when it can not find the file?

evenstensberg

I think find-up is the correct approach here, but your test case doesn't test the implementation. You'd need to create a 2-3 level deep folder and place the file there

lakatostamas · 2018-11-19T16:34:06Z

Thanks for the review!

@hemal7735 Sure, I will add negative cases.

@evenstensberg I think I've added one test which is testing the implementation:
here is the test file (2 level deep: packages/foo): test/binCases/config-location/find-recursively/packages/foo/find-recursively.test.js
and here is the webpack.config.js: test/binCases/config-location/find-recursively/webpack.config.js

I think this will perform the following checks:
1, test/binCases/config-location/find-recursively/packages/foo/
2, test/binCases/config-location/find-recursively/packages/
3, test/binCases/config-location/find-recursively/ (webpack.config.js found)

Should I modify this test or it's completely irrelevant?

Sorry if I totally missed the point, this is my first contribution to any webpack-related package.

hemal7735 · 2018-11-20T15:24:41Z

bin/convert-argv.js

+				return a.concat(i);
+			}, []);
+
+		for (i = 0; i < defaultConfigFileNames.length; i++) {


We will need some logic change here. Let's see both implementations.

existing-implementation: try to find file from ["webpack.config", "webpackfile"] in cwd.

current-modification:

try to find file webpack.config in up-fashion.

if found then return, if not go to step#3

try to find file webpackfile in up-fashion.

if found then return

Do we see the problem here? Even though webpackfile is there one level above, it would not be discovered, but 2 level up webpack.config would be given chance first.

I would want both files to get a fair chance to be discovered. @evenstensberg @ematipico your thoughts on this?

top-down priority is the best

@evenstensberg sorry, I did not get you. Can you give any example?

@hemal7735 I think you are right. I implemented a solution with (dynamically created) RegExp, but I didn't merge it into this PR. I also added webpackfile related tests. Could you please check it? https://github.com/lakatostamas/webpack-cli/compare/1f2b7e2..727a59a

Yeah both files should always have the same priority at each path of directory structure.

bin/convert-argv.js

ematipico

I left some questions and thoughts

ematipico · 2018-11-21T13:31:02Z

bin/convert-argv.js

-		for (i = 0; i < defaultConfigFiles.length; i++) {
-			const webpackConfig = defaultConfigFiles[i].path;
-			if (fs.existsSync(webpackConfig)) {
+		const defaultConfigFileNames = ["webpack.config", "webpackfile"]


This is a good chance to move "webpack.config" and "webpackfile" inside two constants

ematipico · 2018-11-21T13:34:38Z

bin/convert-argv.js

+				return a.concat(i);
+			}, []);
+
+		for (i = 0; i < defaultConfigFileNames.length; i++) {


Yeah both files should always have the same priority at each path of directory structure.

ematipico · 2018-11-21T13:36:24Z

test/binCases/config-location/not-found/not-found.test.js

+	expect(code).toBe(0);
+	expect(stdout).toEqual(expect.anything());
+	expect(stdout).toContain("./src/index.js");
+	expect(stderr).toHaveLength(0);


Apart the ./src/index.js, is there anything else that we should test in the specific (anything() is too generic)? Also, it is worth to have a snapshot test?

expect(stdout).toEqual(expect.anything()); could be stripped

test/binCases/config-location/find-recursively/packages/foo/find-recursively.test.js

…nce (webpack#689)

webpack-bot · 2018-11-23T15:52:00Z

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

lakatostamas · 2018-12-06T07:25:18Z

friendly ping @ematipico @evenstensberg . No rush, but could you please review it again? Thanks! 🙂

evenstensberg

Reviewed, nice job! You will need to fix the broken test cases, they are not passing

evenstensberg · 2018-12-06T10:39:40Z

test/binCases/config-location/find-recursively/packages/foo/find-recursively.test.js

+		"production"
+	]);
+	expect(code).toBe(0);
+	expect(stdout).toEqual(expect.anything());


You should use the extractHash utility.

No, for now hash is already extracted by default. We have to refactor that bit. Will do it with another PR.

@evenstensberg @ematipico So should I use extractHash here? I think I fixed the broken test cases except this one.

evenstensberg · 2018-12-06T10:43:13Z

test/binCases/config-location/find-recursively/packages/foo/find-recursively.test.js

+		"[id].chunk.js",
+		"--target",
+		"async-node",
+		"--mode",


remove prod, delays test case

evenstensberg · 2018-12-06T10:43:31Z

test/binCases/config-location/set-explicitly/set-explicitly.test.js

+		"[id].chunk.js",
+		"--target",
+		"async-node",
+		"--mode",


remove prod

evenstensberg · 2018-12-06T10:44:01Z

test/binCases/config-location/set-explicitly/webpack.dev.js

+
+module.exports = [
+	{
+		name: "foo",


this is wrong

evenstensberg · 2018-12-06T10:44:25Z

test/binCases/config-location/find-recursively/webpack.config.js

@@ -0,0 +1,7 @@
+var path = require("path");
+
+module.exports = [


export object instead

evenstensberg · 2018-12-06T10:44:38Z

test/binCases/config-location/set-implicitly/set-implicitly.test.js

+		"[id].chunk.js",
+		"--target",
+		"async-node",
+		"--mode",


remove prod

evenstensberg · 2018-12-06T10:44:42Z

test/binCases/config-location/set-implicitly/webpack.config.js

+
+module.exports = [
+	{
+		name: "foo",


evenstensberg · 2018-12-06T10:44:48Z

test/binCases/config-location/webpackfile-config/packages/foo/webpackfile-config.test.js

+		"[id].chunk.js",
+		"--target",
+		"async-node",
+		"--mode",


remove prod

evenstensberg · 2018-12-06T10:46:05Z

test/binCases/config-location/webpackfile-config/packages/foo/webpackfile.js

@@ -0,0 +1,7 @@
+var path = require("path");
+
+module.exports = [


At this level, add a webpack.config.js file as well, it shouldn't break the test if implemented correctly.

If I add a webpack.config.js in the same level as webpackfile.js it will break the test because if they are on the same level then webpack.config.js will be used. I've added a webpack.config.js two level above: https://github.com/webpack/webpack-cli/pull/693/files#diff-3f5d33891236c7320bd882814444bca0R1

evenstensberg · 2018-12-06T10:46:32Z

bin/convert-argv.js

-				});
-				break;
-			}
+		const webpackConfigFileRegExp = `(webpack.config|webpackfile)(${extensions.join("|")})`;


configFileRegExp is sufficient

add a constant, so we can add more later, const defaultConfigFiles = ["webpack.config", "webpackfile"] and join them in the regexp instead like the last node in the regexp

…pack#689)

webpack-bot · 2018-12-07T07:09:26Z

@lakatostamas Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@evenstensberg Please review the new changes.

lakatostamas added 2 commits November 18, 2018 18:01

chore(dependencies): add findup-sync (webpack#689)

a972f1d

feat(bin): find config recursively (webpack#689)

14ec0a5

webpack-bot added the PR: unreviewed label Nov 18, 2018

tests(config): add config-location tests (webpack#689)

e941b49

lakatostamas force-pushed the feature/find-config-recursively branch from 7ec1855 to e941b49 Compare November 18, 2018 17:28

evenstensberg requested changes Nov 19, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: unreviewed labels Nov 19, 2018

lakatostamas added 2 commits November 19, 2018 21:55

chore(refactor): use const instead of var (webpack#689)

1bd68a3

tests(config): add negative test-cases to config-location (webpack#689)

1f2b7e2

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Nov 19, 2018

hemal7735 reviewed Nov 20, 2018

View reviewed changes

ematipico reviewed Nov 21, 2018

View reviewed changes

bin/convert-argv.js Show resolved Hide resolved

ematipico suggested changes Nov 21, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: review-outdated labels Nov 21, 2018

lakatostamas added 2 commits November 23, 2018 16:02

fix(bin): find webpackfile and webpack.config file with equal cha…

b61220e

…nce (webpack#689)

tests(bin): add webpackfile related test (webpack#689)

3862295

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Nov 23, 2018

webpack-bot added the PR: CI-ok label Nov 23, 2018

tests(config): remove unnecessary assertion (webpack#689)

1781c1f

evenstensberg requested changes Dec 6, 2018

View reviewed changes

webpack-bot removed the PR: review-outdated label Dec 6, 2018

webpack-bot added the PR: reviewed-changes-requested label Dec 6, 2018

lakatostamas added 3 commits December 7, 2018 08:01

chore(refactor): store config file names in defaultConfigFiles (web…

95dc56c

…pack#689)

tests(config): remove prod mode from config-location tests (webpack#689)

29a6060

tests(config): export objects in config-location tests (webpack#689)

fc8a5b9

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Dec 7, 2018

ematipico approved these changes Dec 25, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Dec 25, 2018

evenstensberg approved these changes Jan 3, 2019

View reviewed changes

webpack-bot added PR: conflict and removed PR: CI-ok labels Jan 3, 2019

chore: resolve conflict

3ec2e9d

webpack-bot removed the PR: conflict label Jan 3, 2019

evenstensberg merged commit f9bb82d into webpack:master Jan 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/find config recursively #693

Feature/find config recursively #693

lakatostamas commented Nov 18, 2018 •

edited by ematipico

jsf-clabot commented Nov 18, 2018 •

edited

hemal7735 commented Nov 19, 2018

evenstensberg left a comment

lakatostamas commented Nov 19, 2018

hemal7735 Nov 20, 2018 •

edited

evenstensberg Nov 20, 2018

hemal7735 Nov 21, 2018 •

edited

lakatostamas Nov 21, 2018

ematipico Nov 21, 2018

ematipico left a comment

ematipico Nov 21, 2018

ematipico Nov 21, 2018

ematipico Nov 21, 2018

evenstensberg Nov 24, 2018

webpack-bot commented Nov 23, 2018

lakatostamas commented Dec 6, 2018

evenstensberg left a comment

evenstensberg Dec 6, 2018

ematipico Dec 6, 2018

lakatostamas Dec 22, 2018

evenstensberg Dec 6, 2018

evenstensberg Dec 6, 2018

evenstensberg Dec 6, 2018

evenstensberg Dec 6, 2018

evenstensberg Dec 6, 2018

evenstensberg Dec 6, 2018

evenstensberg Dec 6, 2018

evenstensberg Dec 6, 2018

lakatostamas Dec 7, 2018

evenstensberg Dec 6, 2018

evenstensberg Dec 6, 2018

webpack-bot commented Dec 7, 2018

		@@ -0,0 +1,7 @@
		var path = require("path");

		module.exports = [

Feature/find config recursively #693

Feature/find config recursively #693

Conversation

lakatostamas commented Nov 18, 2018 • edited by ematipico

Other information Closes #689

jsf-clabot commented Nov 18, 2018 • edited

hemal7735 commented Nov 19, 2018

evenstensberg left a comment

Choose a reason for hiding this comment

lakatostamas commented Nov 19, 2018

hemal7735 Nov 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hemal7735 Nov 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webpack-bot commented Nov 23, 2018

lakatostamas commented Dec 6, 2018

evenstensberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webpack-bot commented Dec 7, 2018

lakatostamas commented Nov 18, 2018 •

edited by ematipico

Other information
Closes #689

jsf-clabot commented Nov 18, 2018 •

edited

hemal7735 Nov 20, 2018 •

edited

hemal7735 Nov 21, 2018 •

edited