Use require instead of requireFromString.

[izaakschroeder]

Although there's some niceness in using the string data straight from the filesystem, you lose the ability to use anything that hooks into the native require call – namely babel and friends. By using vanilla require it becomes possible to use ES6 files for your configuration (or indeed anything else that hooks into standard node module loading machinery). Performance-wise the change should be negligible, given the filesystem/stat cache will be hot from having just loaded the contents of the file.

There's also a check in here to use the default export from ES6 modules if its present.

And it's one less (runtime) dependency.

[izaakschroeder]

/cc @davidtheclark 😄

[davidtheclark]

This looks interest @izaakschroeder, thanks! I will try to find time this weekend to look into it.

[izaakschroeder]

Thanks! It shouldn't be a breaking change. 🎉 I npm link 'd this with postcss to test an ES6 config and it worked exactly as intended. 😄

[izaakschroeder]

You could also modify require-from-string to mimic node's hook system but this will probably be fragile and error prone. Leaving node internals alone always seems like the better choice to me. 😄

[davidtheclark]

Can you add a test where the JS module uses ES2015 syntax? Maybe destructuring and spread operators in the exported object, and also some other unsupported stuff? I'm guessing this will break things, right?

There are also some conflicts to fix and also documentation to add. We should note the new possibilities and also note that reading JS is sync now, so users might get a performance boost by excluding it.

The downside I can see to this is that it is sync instead of async — that was the original reason for using require-from-string. I really wish there were a way around that, but maybe there is not. It's true that the robustness we gain from avoiding require-from-string is probably worth the trouble of require being sync.

[davidtheclark]

I'd like to give this variable and the file more descriptive names. How about importJs?

[davidtheclark]

Does this make the case absolutePath('a/b/foo.config.js'): below unnecessary?

[izaakschroeder]

Can you add a test where the JS module uses ES2015 syntax? Maybe destructuring and spread operators in the exported object, and also some other unsupported stuff? I'm guessing this will break things, right?

✔️ Done. Doesn't break anything in terms of backwards compatibility far as I know.

There are also some conflicts to fix and also documentation to add. We should note the new possibilities and also note that reading JS is sync now, so users might get a performance boost by excluding it.

✔️ Conflicts fixed. If you can give me an example of the docs you want I can add them, otherwise feel free to note it however you like; really the only difference is that people using babel will see things "just work". More of a bug fix than a feature I think.

The downside I can see to this is that it is sync instead of async — that was the original reason for using require-from-string. I really wish there were a way around that, but maybe there is not. It's true that the robustness we gain from avoiding require-from-string is probably worth the trouble of require being sync.

The I/O for one config file (most likely at startup) will probably be negligible; if users are trying to optimize for the 0.05ms it takes to read a config file... there are probably better optimization targets to look out for 😄

Thanks for the feedback! Hope this works! 🎉

[izaakschroeder]

/cc @davidtheclark ^

[davidtheclark]

The test with ES2015 syntax uses Babel ... I guess I poorly explained the point. We can't advertise that you can just use ES2015 modules: you can only do so if your Node application is compiled with Babel, right? We'll need to hash out the specific way to explain this in the docs. Since it's a matter of documentation, let's remove the test and additional dependencies.

Regarding docs: any references to async in the README need to be qualified or removed. And references to JS exports should note that ES2015 modules can be used but cosmiconfig does not compile the source to ES5 — the consumer app needs to handle the ES2015.

After those additions to the docs — and sorry about the flip-flopping on the test — let's merge and release.

[izaakschroeder]

Regarding docs: any references to async in the README need to be qualified or removed. And references to JS exports should note that ES2015 modules can be used but cosmiconfig does not compile the source to ES5 — the consumer app needs to handle the ES2015.

✔️ Done!

All yours. 🎉

[davidtheclark]

@izaakschroeder Dang, after merging I realized that there are lots of problems with the tests. I can try to finish this up, but the most important question is this: mock-require does not seem to provide spy functionality — I can't see what was called — but that's essential to the tests. Do you know of another approach? Looks like proxyquire might be promising.

[davidtheclark]

@izaakschroeder I reverted this merge. Sorry for the confusion. Please open another PR from your branch, but there are some essential changes necessary for the tests to work:

• fs should never try to read JS files. The fact that it does is a bug in loadJs.
• No JS files should be listed in the switch statement for stubs of readFile, because they won't get read. This will affect all of the counts and assertions related to those stubs.
• importJs needs to be stubbed similarly (maybe that means the file needs to export an object), such that we can check to see how many times it was called, with what filenames. Something like this:

importJsStub = sinon.stub(importJs, 'import', function (searchPath) {
      switch (searchPath) {
        case absolutePath('a/b/foo.config.js'):
        case absolutePath('a/foo.config.js'):
        case absolutePath('foo.config.js'):
          throw new Error({ code: 'MODULE_NOT_FOUND' });
      }
});

Sorry, you probably thought this wasn't a huge change, but it really is fundamentally modifying the way that JS files get loaded, so will involve a lot of adjustments. If you are not able to do this, I can try to find some time this weekend.

[izaakschroeder]

What do you mean "some essential changes necessary for the tests to work"? The build passed just fine previously - which tests are failing?

fs should never try to read JS files. The fact that it does is a bug in loadJs.

Yeah it probably shouldn't. It won't break anything, other than being an extremely minor performance annoyance. (Basically just heating cache which the next require call will pick up on).

No JS files should be listed in the switch statement for stubs of readFile, because they won't get read. This will affect all of the counts and assertions related to those stubs.

Indeed if the former change is made then an update here becomes a necessary concession.

importJs needs to be stubbed similarly (maybe that means the file needs to export an object), such that we can check to see how many times it was called, with what filenames.

What test does this need to be stubbed for? But you are correct, sinon can't stub default exports; so giving it a name will allow sinon to stub it.

[davidtheclark]

Yes, tests were passing; but they were not testing the right things, so it doesn't matter that they were passing. They need to be changed if we are to integrate this new feature. Almost all the tests need to be changed, because they all were testing JS files read via fs instead of require.

fs reading JS files is unquestionably a bug that needs to be fixed. Any unnecessary file read would be a bug.

(Re "heating cache" I don't think that's going to happen: In this case we will later read the file again with require but I do not think there is any caching relationship between fs reads and require (or even between on fs read and the next).)

The importJsStub example would need to be mimicked in all of the tests that stub fs. We are not using fs anymore for JS, but still need to test, in every case:

• Were the correct files required?
• And only the correct files?
• When a require fails do we do the right thing?
• When a require succeeds do we do the right thing?