Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESM support #29

Merged
merged 10 commits into from
May 9, 2020
Merged

ESM support #29

merged 10 commits into from
May 9, 2020

Conversation

giltayar
Copy link
Contributor

@giltayar giltayar commented May 1, 2020

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

@giltayar giltayar changed the title WIP: ESM support [Do not merge] ESM support May 2, 2020
@searls
Copy link
Member

searls commented May 4, 2020

Hi @giltayar! Did you by chance see this PR on teenytest by @jkrems? testdouble/teenytest#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
Copy link
Contributor Author

giltayar commented May 4, 2020

@searls Funnily enough, I was the one that implemented ESM for Mocha (mochajs/mocha#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
Copy link
Member

searls commented May 4, 2020

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
Copy link
Contributor Author

giltayar commented May 4, 2020

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
Copy link
Contributor Author

giltayar commented May 4, 2020

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

@giltayar
Copy link
Contributor Author

giltayar commented May 8, 2020

@searls I replaced Mocha with teenytest.

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

@searls
Copy link
Member

searls commented May 9, 2020

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

@giltayar
Copy link
Contributor Author

giltayar commented May 9, 2020

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
Copy link
Contributor Author

giltayar commented May 9, 2020

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

@giltayar
Copy link
Contributor Author

giltayar commented May 9, 2020

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

@searls
Copy link
Member

searls commented May 9, 2020

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 searls merged commit 9c71866 into testdouble:master May 9, 2020
@searls
Copy link
Member

searls commented May 9, 2020

Landed in 0.6.0

@giltayar
Copy link
Contributor Author

giltayar commented May 9, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants