testdouble / quibble

Makes it easy to replace require'd dependencies.
93 stars 26 forks source link

Add support for esm #24

Closed pingortle closed 4 years ago

pingortle commented 5 years ago

What does this do?

Add support for libraries and apps using esm, an ES6 module loader for node.

Status

Works On My Machine™ when testing with npm link in this repo: https://github.com/pingortle/quibble-plus-esm

To do

See https://github.com/testdouble/quibble/issues/13

searls commented 5 years ago

Apologies for taking so long to review this. Three areas of questions for me

  1. So that I understand, does this PR's esm-example actually use esm at all? It seems like it doesn't, so I'm not sure what it's proving (other than "don't blow up when esm is in my package.json"). Wouldn't it be more illustrative to use import and export in there

  2. What's the adoption on esm look like? How graceful a bridge is it turning out to be for Node core esm modules? This is one area where I tend to not pay much attention in Node-land. What I'm driving at: is there large enough adoption of esm or such good compatibility that it buys us and users enough to overcome any confusion caused by support for "real" es import/export in node? Maybe @jasonkarns or @rosston have a take on this?

  3. I added a comment about the resolve.sync call's paths option, and I honestly don't know what paths should be set to so that it always works when a test is being run by a project that has esm loadable somewhere/somehow. In the past to be sure I'll create three or four example projects with different configurations to test this:

    • One with esm in the package and a test run with CWD at the top level
    • One with esm in the package and a test run with CWD at the top level
    • One with esm loaded transitively but not flattened at the top node_modules (like, buried in a recursive node_modules directory. I know npm and yarn both flatten, but I've never felt comfortable with taking it as a guarantee)
    • One with esm loaded transitively, in a buried node_modules, and with a test running from a subdirectory
pingortle commented 5 years ago

@searls Thanks for the review! You raise some excellent concerns I hadn't considered with the transitive esm dependency case. I will spend a bit of time looking into the various permutations you listed. 👀

So that I understand, does this PR's esm-example actually use esm at all?

Actually it does, but it is super not obvious. In esm-example/package.json I run mocha with -r esm in the test script. If we are 👍 on the general idea of nested example packages to test this feature, I'd like to use ES modules more explicitly as you suggest.