Resolve hook source transformation

[coreyfarrell]

I've created a proof of concept for adding source transformation support to the ESM loader resolve hooks. An example --loader script using this feature:

import {fileURLToPath} from 'url';
import babel from '@babel/core';

async function fetchSource(url, defaultFetchSource) {
  const source = await defaultFetchSource(url);
  const {code} = await babel.transformAsync(source, {
    // Protect against `data:` url
    filename: url.startsWith('file:') ? fileURLToPath(url) : undefined,
    sourceMaps: 'inline'
  });

  return code;
}

export async function resolve(spec, parentMod, baseResolver) {
  const result = baseResolver(spec, parentMod);
  if (result.format !== 'module') {
    return result;
  }

  return {
    ...result,
    fetchSource
  };
}

Thoughts on this API? Currently my patch supports the fetchSource option for module, wasm and json formats. I'm unsure what the return value of fetchSource should be. Currently a string is expected because is was easiest to implement and matches the existing getSource, but maybe it would make sense for it to allow an object {source, sourceMap}? To do that I'd need advice on what to do with the sourceMap object.

[devsnek]

transformSource can be generalized to a fetch/load hook, which would also be more generic in terms of non-js esm like wasm and such.

[coreyfarrell]

@devsnek agreed, I've updated my proposal to specify fetchSource. The defaultFetchSource argument to fetchSource is the current getSource function from lib/internals/modules/esm/translators.js, this way it will be easy to implement transform-only without changing default load behavior. I haven't created docs or tests for this yet so no PR but the branch is posted to https://github.com/coreyfarrell/node/tree/esm-transform if anyone wants to take a look (it's actually a small patch).

If a fetchSource is used then file: / data: URL is not strictly required. This allows http(s) or application specific import URL's to be supported through loader hooks. Pointless example:

if (specifier === 'myapp://testing') {
  return {
    url: specifier,
    format: 'module',
    fetchSource() {
      return 'export function testStack() {\n  throw new Error("stack trace test");\n};';
    }
  };
}

Importing 'myapp://testing' and calling this testStack function resulted in the desired output:

myapp://testing:2
  throw new Error("stack trace test");
        ^

Error: stack trace test
    at testStack (myapp://testing:2:9)
    at file:///usr/src/public/node/loader-exp/x.js:8:1

[bcoe]

@nodejs/tooling this seems like it would enable a lot of behavior currently supported by require.extensions.

[coreyfarrell]

I thought about this some more and I think fetchSource and transformSource need to be separate. Combining them would block the ability to have multiple --loader arguments each providing an incremental transformation.

I've opened nodejs/node#29924 with my current implementation including a basic test. In the test I just used string replacement on the source to provide multiple transforms ran in the expected order. I've tested the updated hook locally using babel:

import {fileURLToPath} from 'url';
import babel from '@babel/core';

export async function transformSource(url, source) {
  const {code} = await babel.transformAsync(source, {
    filename: url.startsWith('file:') ? fileURLToPath(url) : undefined,
    sourceMaps: 'inline'
  });

  return code;
}

[guybedford]

I could get behind landing an experimental 'fetch' hook in the current loader API in order not to block this feature on the new loader overhaul (as it basically is currently). Whether that would be blocked by other collaborators I'm not sure though but it could be worth a try.

I would prefer not to separate fetch and transform though as we start making it seem like we are encouraging loader composition then too and this goes down a path of eventually becoming the standard way of doing things by default.... which we want to avoid - this is a stopgap.

[SMotaal]

I feel inclined to agree for now fetch being a good place to start without making any premature assumptions… But would be worth discussing any concerns with fetch doing one thing today that may or may not change down the road.

So it would be experimental and only that one aspect to worry about, but do we have interface-related concerns... etc?

[guybedford]

The new loader designs are already a completely new API and interface that would be a breaking change. So we will break the entire API currently regardless! Hence we do have flexibility still.

[SMotaal]

Great, so outdated loaders are going to always fail… gotcha 👍

[coreyfarrell]

I could get behind landing an experimental 'fetch' hook in the current loader API in order not to block this feature on the new loader overhaul (as it basically is currently). Whether that would be blocked by other collaborators I'm not sure though but it could be worth a try.

fetch does something fundamentally different from transform. As you know I'm looking at this from the prospective of nyc. It's not nyc's place to decide how/where the source comes from, only to ensure that coverage counters get injected into the source so we get coverage counters. This same argument applies to babel and I assume other tools.

Side question 'new loader overhaul' - can you point me to issues, PR's or other links about this?

I would prefer not to separate fetch and transform though as we start making it seem like we are encouraging loader composition then too and this goes down a path of eventually becoming the standard way of doing things by default.... which we want to avoid - this is a stopgap.

Not sure I understand what you mean by this, your suggesting that supporting multiple transform hooks should never be allowed?

[guybedford]

@coreyfarrell I'm simply mentioning a simple option to move forward for you easily with your use case, because you mentioned this in the meeting and I thought I could help by offering a suggestion, that is all.

Because each hook (like resolve) takes the default implementation as an argument a fetch hook that chains the default fetch is effectively a transform hook, without needing to know anything about how the default implementation behaves.

I tend towards the simple fetch-hook-only suggestion just because that would be easiest for implementation and consensus. But if you want to try push for something else you are welcome to.

Re new loader API designs, there are a bunch of documents. @bmeck would be the person to chat to about that.

[coreyfarrell]

Closing as this is resolved.