Esm: Question regarding require hooks

[clintwood]

I'm using flow-remove-types/register which is a require hook to strip flow annotations before running in node. It works really well and avoids a build step.

I now want to use @std/esm as well like this, e.g.: node -r flow-remove-types/register -r @std/esm ./index.mjs. But it seems @std/esm is not respecting a previously installed require hooks/loaders. I have looked into flow-remove-types/register and it is correctly calling prior loaders.

I have looked through @std/esm and can't quite figure out if it is calling prior hooks/loaders - should it be doing so?

[jdalton]

Hi @clintwood!

Could you create a simple repro repo for me to look at?

[clintwood]

Hey @jdalton, sorry for the delay here is a repo which illustrates the issue.
To repro (please see scripts in package.json):

# clone
git clone https://github.com/clintwood/flow-std-esm
# install 
npm i
# build
npm run build
# works - without -r @std/esm but with -r flow-remove-types/register (with flow but without esm import)
npm run with-flow-no-esm
# works - with -r @std/esm but without -r flow-remove-types/register (without flow but with esm import)
npm run with-flow-removed
# fails - with -r @std/esm first, -r flow-remove-types/register second (with flow and esm import)
npm run with-flow-1
# fails - with -r flow-remove-types/register first, -r @std/esm second (with flow and esm import)
npm run with-flow-2

Hope this helps!

[jdalton]

Hi @clintwood!

After digging into this a bit more support for flow-remove-types becomes tricky. It wraps in a way that we can't tie-in to. It would be rad if they used pirates (what babel-register and nyc use) so that there's one kind of hook to target support for. You should open an issue on them to adopt it.

[clintwood]

Hey @jdalton, will look into this with them - thanks for spending time on it!

[clintwood]

Hey, @jdalton, I decided to prepare for a PR to flow-remove-types that switches their require hook logic to use pirates as you suggest above. The same issue exists.

Here is the new repo on the pirates-require-hook branch that you can look at to repro as above.

From my observation, even though flow-remove-types is properly registering it's require hooks, once @std/esm registers it's hooks, flow-remove-types hook logic is never called again.

The error I get shows that the flow annotations are not removed producing a compile error - that tells me that somehow _a loader_ is loading and _compiling_ before getting toflow-remove-types hook logic.

Also, I now see that pirates will not work in this case since flow-remove-types switches module._compile to afford it the opportunity to remove the annotations. I assume @std/esm must be doing the same - so I'd suggest debugging using the originally submitted master branch of the repro repo for this issue as I believe the original hook logic in flow-remove-types looks correct.

[clintwood]

Hey @jdalton, just for my own sanity I've eliminated flow-remove-types and updated the repro repo pirates-require-hook branch with two additional npm run scripts: npm run pirates-hook-1 and npm run pirates-hook-2 which uses a repro package I have created clintwood/pirates-hook which simply logs what is being hooked/required (code, filename) and passes code through using pirates.

From my tests the pirates-hook hook is never called!

# clone
git clone https://github.com/clintwood/flow-std-esm.git
git checkout pirates-require-hook
# install 
npm i
# build
npm run build
# pirates-hook registered and called: node -r pirates-hook ...
npm run pirates-hook-no-esm
# pirates-hook registered but never called: node -r @std/esm -r pirates-hook ...
npm run pirates-hook-1
# pirates-hook registered but never called: node -r pirates-hook -r @std/esm ...
npm run pirates-hook-2

EDIT: added pirates-hook-no-esm npm run script to show pirates-hook working.

[jdalton]

@clintwood

Oh, ya switching it to pirates won't fix std/esm, but it will simplify the work needed to be done 😋

[jdalton]

@clintwood

Looking forward to your flow-remove-types PR!

[clintwood]

@jdalton
flow-remove-types#PR53 has been there for a few days - it at least enables flow-remove-types to use then .mjs extension for starters - lets see if it gets accepted - @leebyron?

[jdalton]

Hey @clintwood!

Just a heads up that I patched https://github.com/standard-things/esm/commit/6c11c7f5b33361c925da7af3919a6a176588fe0b @std/esm to work with the style of hooks that pirates provides. This means @std/esm will work with babel-register@next and others that use it. If you're still up for making the PR for flow-remove-types that would rock.

\cc @danez

[anandthakker]

Submitted a PR for this here: https://github.com/flowtype/flow-remove-types/pull/62