testdouble / quibble

Makes it easy to replace require'd dependencies.
94 stars 25 forks source link

ESM support #29

Closed giltayar closed 4 years ago

giltayar commented 4 years ago

Node.js Native ES module support for quibble

Finished implementing and testing quibble support for Node.js ESM. The support adds two functions:

  1. quibble.esm: an async function that enables mocking named exports (second argument) and default export (third argument) in the same way quibble does. See below for reasons why this function is async.
  2. quibble.esmImportWithPath: an async function that imports the module, but also returns the path to it. Will be used in the testdouble.js implementation to get at the named/default exports so they can be imitated (see below for proposed implementation of td.replaceEsm.

The reason that quibble.esm is that I found a really cool way of resolving the real path of an import (this works only if you're a loader), and it works only if you "dummy import" the module, which needs to be async. But I believe this is fine because td.replaceEsm needs to be async anyway due to it having to import the module (see draft code below).

Draft code for testdouble.js implementation of td.replaceEsm:

export async function replaceEsModule(path, namedExportsStub, defaultExportStub) {
  if (arguments.length > 1) {
    return await quibble.esm(path, namedExportsStub, defaultExportStub)
  }

  const {modulePath, module} = await quibble.esmImportWithPath(path)
  const {fakeDefaultExport = undefined, ...fakeNamedExports} = imitate(module, fakeName(modulePath, module.default))

  await quibble.esm(modulePath, fakeNamedExports, fakeDefaultExport)

  return fakeThing
}

Notes for PR

  1. Removed Node v6 from travis (to enable me to support async/await), and added 12, 13, 14.
  2. Upgraded standard to latest to support async/await and import.
  3. Tried to work around teenytest lack of support for --loader, failed (bugs will be opened in Node.js), and just installed mocha (and chai with it)
  4. Added example-esm with same functionality as example
  5. No ESM support for config({defaultFakeCreator}) (not used in testdouble.js)
  6. I updated the docs too.

Fixes #24

searls commented 4 years ago

Hi @giltayar! Did you by chance see this PR on teenytest by @jkrems? https://github.com/testdouble/teenytest/pull/53

Since you are many steps ahead of me in understanding how Node's ESM support works, if you're game to help make teenytest support what you need, I'd really really appreciate it (I'd be loathe to add a second test runner to this project)

giltayar commented 4 years ago

@searls Funnily enough, I was the one that implemented ESM for Mocha (https://github.com/mochajs/mocha/pull/4038), and it was not easy there, but I guess it could be easier in teenytest, due to it being... teeny.

I looked at the PR, and interestingly enough, it uses a similar approach that I took in loading ESM: require each module, but if you get an ESM failure, import it. The gotcha is that all the path up to the loading of the modules now has to be async. That was most of the Mocha work.

But I'm game if you're game, and if @jkrems is OK with me continuing with the PR. Assuming it's OK to drop Node versions <8 (which it should be, because they are officially not supported anyway).

I'd love it if you could look at the rest of the code, and the changes I noted in the PR description to see if you're OK with them.

In any case, I've already started working on testdouble to use the new quibble.

searls commented 4 years ago

Hahah, I'm sure he's ok since he expressly said it was a starting point to help us down that direction. I'm also fine with dropping non-LTS Nodes. Thank you for looking at it!

I did a quick review of the code and I think it looks great. I appreciate how thorough you were in mimicking the existing style and shape of the codebase

giltayar commented 4 years ago

Thanks. Will start sometime this week.

(As for code-style. You wouldn't believe how hard it is for me to use var-s and ;! 😉)

giltayar commented 4 years ago

BTW, there are a lot of cool "loader" techniques I used in that codebase. Definitely worth a blog post somteimes.

giltayar commented 4 years ago

@searls I replaced Mocha with teenytest.

But! To make the tests pass you need to merge the PR in teenytest (https://github.com/testdouble/teenytest/pull/57) and publish the module (which it itself is dependenant on another PR https://github.com/testdouble/function-names-at-line/pull/3 which needs to be merged and published)

searls commented 4 years ago

Ok! now that I've published the two depended libraries, try updating this one to your liking?

giltayar commented 4 years ago

I see we have a problem with Node v8, because import is a syntax error (and not a runtime one), so it can't even load the files. I had the same problem in Mocha, and I'll fix it (by conditionally requiring a module that has the "import" functionality) in a sec.

giltayar commented 4 years ago

Oh, no. Wait. The problem is also in teenytest. Do we want to support Node v8?

giltayar commented 4 years ago

@searls: for this to pass, you need to merge (and publish) another PR: https://github.com/testdouble/teenytest/pull/58 (this brings back to teenytest support for Node v8, which I broke inadvertently)

searls commented 4 years ago

Ok, teenytest@6.0.1 should fix you. Would have hand-merged this but wasn't 100% sure you thought it was ready to go

p.s. @pingortle, I flagged this as obviating your #24 PR

searls commented 4 years ago

Landed in 0.6.0

giltayar commented 4 years ago

Woopeee! I've started working on testdouble.js. Looking good, but I'll need a few more days.