feat: Discover config files in home dir, working dir #1161

kumar303 · 2017-12-15T17:21:27Z

Fixes #730

coveralls · 2017-12-15T17:25:48Z

Coverage remained the same at 100.0% when pulling 99358ed on kumar303:discover-configs-730 into f4ff99a on mozilla:master.

rpl

@kumar303 follows a round of review comments (mostly nits and small tweaks on the implementation and new tests)

I was also wondering if it could make sense to apply the tweaks to the flow type annotations related to the global and command options as part of this pull request (e.g. as described in #1144), especially if it doesn't require a big amount of changes to make it flow happy with the "cli options type checking".

I don't see it as a mandatory change for this pull request, but it could be nice addition, given that once we merge this pull request the config file support seems pretty complete (and, as previously discussed, a more precise flow type annotation of the options could be useful to prevent regressions related to the config file loading when we add new command line options).

rpl · 2017-12-20T14:50:10Z

src/config.js

  argv: Object,
+  // This is the argv that only has CLI values applies to it.


Nit, s/applies/applied/

rpl · 2017-12-20T14:59:06Z

src/config.js

+    // Look for a magic hidden config (preceded by dot) in home dir.
+    path.join(getHomeDir(), `.${magicConfigName}`),
+    // Look for a magic config in the current working directory.
+    path.join(process.cwd(), magicConfigName),


Not an issue (just a proposed "nice to have" for a follow up):

we could also support config options loaded from a section in the package.json file (e.g. a "web-ext" section, similarly to how other nodejs tools support config loading from the package.json file, as an example babel and jest both support it).

Good idea. Filed: #1166

rpl · 2017-12-20T15:05:28Z

src/config.js

+        log.debug(
+          `Discovered config "${resolvedFileName}" does not ` +
+          'exist or is not readable');
+        return false;


uhm... I'm surprised that flow is not complaining here :-)

This async arrow function is basically returning a value of type Promise<string | boolean>, I would probably prefer if we return undefined when the file does not exist, so that the returned type is a "Maybe type" like Promise<?string>.

rpl · 2017-12-20T15:09:27Z

src/config.js

+  ));
+
+  const existingConfigs = [];
+  configs.forEach((f) => {


If we opt for the Maybe type Promise<?string>, we have to change if (f) { ... } into if (typeof(f) === 'string') {...}.

Also how about using configs.filter and returning its result (return configs.filter((f) => typeof(f) === 'string');) so that we don't need to accumulate them in the new existentConfig array?

I'll take another look but I think filter() was failing because Flow doesn't seem to understand that the returned array will not contain undefined values (or something like that).

This is the best workaround I found:

const files = ( (configs.filter((f) => typeof f === 'string'): Array<any>): Array<string> ); return files;

That's too confusing though. I think it's probably best to leave it as is.

The Flow bug: facebook/flow#1414

yeah, I agree, better to leave it as it is (which is also probably much more readable to a contributor that is not so used to the flow syntaxes).

rpl · 2017-12-20T15:10:49Z

src/program.js

@@ -37,6 +39,7 @@ type ExecuteOptions = {
  loadJSConfigFile?: typeof defaultLoadJSConfigFile,
  shouldExitProgram?: boolean,
  globalEnv?: string,
+  discoverConfigFiles?: typeof defaultConfigDiscovery,


Nit, I would prefer it near line 39 or 38.

rpl · 2017-12-20T16:14:47Z

tests/unit/test.config.js

@@ -474,6 +505,40 @@ describe('config', () => {
      assert.strictEqual(newArgv.apiKey, cmdApiKey);
    });

+    it('can load multiple configs', () => {


This test looks pretty similar to the one at line 252, but on the sign command instead of a fake command, aand with commantOpt instead of globalOpt.

if we need both these tests, it would be nice to extract the common part into a test helper function defined in this file.

Yeah, they are both necessary. One will apply a global option twice and the other will apply a sub-command option twice. The sub-command test covers some recursion.

I added some comments and improved the test specs to make it more clear. I couldn't really see any code that could be abstracted in a helpful way.

👍 the comments and the changes to the test names are useful and more then enough to quickly notice how the two tests are different in purpose (so that we don't end up to remove them by mistake by missing their different purpose), looks good to me.

rpl · 2017-12-20T16:23:39Z

tests/unit/test.program.js

+      },
+    };
+    // Instead of loading/parsing a real file, just return an object.
+    const fakeLoadJSConfigFile = sinon.spy(() => {


This is defined as a sinon.spy, we could use it to assert that it is called with the expected filename, am I right?

Good catch, fixed

rpl · 2017-12-20T16:24:29Z

tests/unit/test.program.js

+      {
+        commands: fakeCommands,
+        runOptions: {
+          discoverConfigFiles: async () => ['fake/config.js'],


we could also wrap this discoverConfigFiles into a sinon.spy and check that it has been called as expected.

I prefer to check things like that implicitly when possible. For this test case, if discoverConfigFiles is not called, you get this error:

1) program.main discovers config files: AssertionError: expected false to equal true at _callee5$ (dist/webpack:/tests/unit/test.program.js:556:12) at tryCatch (node_modules/regenerator-runtime/runtime.js:62:40) at GeneratorFunctionPrototype.invoke [as _invoke] (node_modules/regenerator-runtime/runtime.js:296:22) at GeneratorFunctionPrototype.prototype.(anonymous function) [as next] (node_modules/regenerator-runtime/runtime.js:114:21) at step (dist/webpack:/node_modules/babel-runtime/helpers/asyncToGenerator.js:17:1) at dist/webpack:/node_modules/babel-runtime/helpers/asyncToGenerator.js:28:1

When you look up the line of the assertion failure you will see this:

// This makes sure that the config object was applied // to the lint command options. assert.equal( options.selfHosted, configObject.lint.selfHosted);

I think that will be helpful to diagnose the problem.

rpl · 2017-12-20T16:25:51Z

tests/unit/test.program.js

+      lint: () => Promise.resolve(),
+    });
+
+    const discoverConfigFiles = sinon.spy(() => Promise.resolve([]));


Not mandatory, but we could do the same for loadJSConfigFile (defining it as a sinon.spy and assert that it has never been called).

The test you commented on is just about disabling config discovery. I think it's enough coverage to just assert that the discovery function is not called. Without that call, there is no way to load a discovered file.

yeah, I was thinking more about a possible "future regression" where some new code could try to call loadJSConfigFile without having called discoverConfigFile first, but that's more likely to be just "me being too paranoid" :-P

rpl · 2017-12-20T16:37:59Z

tests/unit/test.program.js

+      }
+    );
+
+    const options = fakeCommands.lint.firstCall.args[0];


This tests should test that the config file are loaded and applied in the expected order, how about wrapping loadJSConfigFile in a sinon.spy and assert that it has been called with the filenames in the expected order?

Oh, yeah, thanks. This test wasn't checking the order at all. Fixed.

kumar303 · 2017-12-21T16:09:47Z

I was also wondering if it could make sense to apply the tweaks to the flow type annotations related to the global and command options as part of this pull request

I think we should defer this because I tried it and fell down a rabbit hole. It's a tough change because the test suite needs a ton of fixes to pass the right default CLI params to each command callback. This is something we should have done all along, whoops.

coveralls · 2017-12-21T17:43:42Z

Coverage remained the same at 100.0% when pulling 272b961 on kumar303:discover-configs-730 into f4ff99a on mozilla:master.

feat: Discover config files in home dir, working dir

99358ed

kumar303 requested a review from rpl December 15, 2017 17:21

rpl requested changes Dec 20, 2017

View reviewed changes

Fixes from feedback

272b961

kumar303 requested a review from rpl December 21, 2017 17:37

rpl approved these changes Dec 21, 2017

View reviewed changes

kumar303 merged commit 649c1c2 into mozilla:master Dec 21, 2017

kumar303 deleted the discover-configs-730 branch December 21, 2017 22:13

Rob--W mentioned this pull request Jul 20, 2023

Feat: support .web-ext-config.js in the current directory. #2812

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Discover config files in home dir, working dir #1161

feat: Discover config files in home dir, working dir #1161

kumar303 commented Dec 15, 2017

coveralls commented Dec 15, 2017

rpl left a comment

rpl Dec 20, 2017

rpl Dec 20, 2017

kumar303 Dec 21, 2017

rpl Dec 20, 2017

rpl Dec 20, 2017

kumar303 Dec 20, 2017

kumar303 Dec 21, 2017

rpl Dec 21, 2017

rpl Dec 20, 2017

rpl Dec 20, 2017

kumar303 Dec 21, 2017

rpl Dec 21, 2017

rpl Dec 20, 2017

kumar303 Dec 21, 2017

rpl Dec 20, 2017

kumar303 Dec 21, 2017

rpl Dec 20, 2017

kumar303 Dec 21, 2017

rpl Dec 21, 2017

rpl Dec 20, 2017

kumar303 Dec 21, 2017

kumar303 commented Dec 21, 2017

coveralls commented Dec 21, 2017

		argv: Object,
		// This is the argv that only has CLI values applies to it.

feat: Discover config files in home dir, working dir #1161

feat: Discover config files in home dir, working dir #1161

Conversation

kumar303 commented Dec 15, 2017

coveralls commented Dec 15, 2017

rpl 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

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

kumar303 commented Dec 21, 2017

coveralls commented Dec 21, 2017