module: remove dynamicInstantiate loader hook

[jkrems]

The dynamicInstantiate loader hook requires that the hooks run in the
same global scope as the code being loaded. We don't want to commit to
this being true in the future. It stops us from sharing hooks between
multiple worker threads or isolating loader hook from the application
code.

Using getSource and getGlobalPreloadCode the same use cases should
be covered.

Checklist

• make -j4 test passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[jkrems]

/cc @nodejs/modules-active-members

[devsnek]

this isn't needed

[jkrems]

Yeah, I started without it. But then the test still passed and I was super confused if anything happened. Having the warning did help me be confident that it actually ignored the hook.

That being said - happy to remove this last trace as well.

[jkrems]

The only reason for needing this is because this hook is messing with the builtin module module. Otherwise the generated code could import {createRequire} from 'module' without needing a global.

[guybedford]

If we are going to push users into this hook now, then I think we should aim to work on this usability first - you shouldn't need to define a secret global for the common use case...

[devsnek]

this seems horrible enough to constitute figuring out some sort of replacement for dynamicInstantiate

[jkrems]

It's the hackiest version of generating the code. I don't agree that the loader hooks providing proper JS code is horrible in general. It's possible to create a dynamicInstantiate-like API on top of this primitive.

[guybedford]

Thanks, I'm approving this PR, but on the condition that we put some thought into the new hook usability before landing.

[guybedford]

If we are going to push users into this hook now, then I think we should aim to work on this usability first - you shouldn't need to define a secret global for the common use case...

[bmeck]

I've been trying to review what the ecosystem has been doing with loaders in our branch before making any sudden movements. I'd agree that for now they are using globals (and writing blog posts about it). I think we definitely need a 1st class communications channel between modules and getPreloadCode but I'm unclear on what it would look like. However, there don't appear to be needs for everyone to use dynamic instantiate to achieve workflows and some appear to be avoiding the hook. I think we might want to figure out a communications channel before landing this PR, but I would not want to block on solving the communications channel issue.

[bmeck]

LGTM pending if we solve first class values to comms between getGlobalPreloadCode and generated code in a follow up.

[devsnek]

I'd like us to have some definitive way of creating modules from data (not transformed code) that doesn't require generating javascript source text.

[jkrems]

That sound like it would require a new format then (“data”?). That seems possible as long as we specify it as transferable data.

[devsnek]

@jkrems well it is a bit more complex, you're generally working with live values, for example webassembly instances, a kind of data you might want to expose as an es module, can't be transferred.

[jkrems]

One thing I would say, in a world where loaders start being integrated: I’m starting to think that our window is closing for a change like this, experimental status or not. So a possible outcome here would be that we give up on scope isolation and loaders are just stuck running in the app context.

[jkrems]

@devsnek I assume you can’t instantiate a WebAssembly module from outside the isolate. So is that a general objection to shared loaders?

[devsnek]

@jkrems i don't object to shared loaders, but i'm not a fan of removing unshared loaders.

[jkrems]

I’m not sure I follow - as in: some loaders would run in one isolate but then others wouldn’t run in that isolate. How would that compose? If the typescript loader is shared and makes assumptions based on being shared, would it be impossible to use it in an app that also needs a non-shared loader? I can’t think of a layout where we could have both without it being pretty confusing. I think we’d have to make a choice on how loaders work, otherwise I’m afraid of a complexity explosion.

[devsnek]

@jkrems i don't think it makes that much complexity, aside from having to choose where the loader you're writing runs. from the perspective of the node internal loader it's just a bunch of loader.resolveOrWhatever calls which might communicate with a worker thread or call a function directly.

[jkrems]

But if losers compose, their return values get chained. So a loader that runs after a non-shared loader has to be non-shared. Otherwise it wouldn’t be able to access the return value of its parent that ran on a different isolate.

[devsnek]

@jkrems you shouldn't be able to chain the result of hooks which create actual module instances, regardless of where they run, there's no composition you can do at that point.

[jkrems]

It would create a situation where loaders have to choose between mutually exclusive modes which means that loaders can’t be directly combined in userland. And it means that a loader that wants to proxy/instrument others can’t be shared. As I said: I’d consider that situation pretty bad UX. I think it would effectively mean that shared loaders are DOA because there’s always a risk that weeks into development of a shared one it would turn out that compat with a non-shared loader forces to abandon sharing.

So, at least from perspective I wouldn’t want to make loaders even more complicated for something that would be a foot gun to users. So if there are objections to making loaders exclusively shared, I would rather invest time in making non-shared loaders better than to tack on a semi-supported shared mode. It’s unfortunate for resource usage etc but maybe that can be worked out in userland.

[devsnek]

@jkrems i don't understand at all why allowing this means loaders can't be composed. can you expand on that point?

[jkrems]

Let’s say I want to write a userland package that allows various transformation and instrumentation. The order of each of the things it’s trying to do is nice and linear. But some of the tasks are implemented in packages written as shared loaders, some in packages written as non-shared loaders. I would somehow have to tell node to run this combined/composed loader as both a shared and a non-shared loader. Actually, likely multiple times so I can actually run the interleaved steps in the correct mode. I don’t think this is acceptable UX.

[devsnek]

@jkrems i think the loaders say whether they are shared or not, you just pass --loader x --loader y like normal.

as both a shared and a non-shared loader

not sure what you mean here, i'm imagining they're one or the other

[jkrems]

Landed in d12d5ef

[EntraptaJ]

I'm unable to recreate my CommonJS exporter system in getSource or getGlobalPreloadCode I was using dynamicInstantiate to support deconstructed imports in ESM code of CommonJS modules, which I now can't find another way of doing. Meaning my TS-ESNode is no longer a drop in replacement, leading to the adoption of ESM Modules not being as simple as I was able to with pre Node 14.4

export async function dynamicInstantiate(
  url: string,
): Promise<DynamicInstantiateResponse> {
  const urlParts = url.split('/node_modules/');

  // Extract the module name after node_modules.
  const moduleName = urlParts.pop()!;

  // With NPM, this is just top-level node_modules.
  // With PNPM, this is the innermost node_modules.
  const nodeModulesPath = urlParts.join('/node_modules/');

  // Create a require function next to node_module, and import the CommonJS module.
  const require = createRequire(`${nodeModulesPath}/noop.js`);
  let dynModule = require(moduleName);

  // Adapt to default exports in CommonJS module.
  if (dynModule.default && dynModule !== dynModule.default) {
    dynModule = {
      ...dynModule.default,
      ...dynModule,
    };
  }

  // Export as ES Module.
  const linkKeys = Object.keys(dynModule);
  const exports = dynModule.default ? linkKeys : [...linkKeys, 'default'];
  return {
    exports,
    execute: (module): void => {
      module.default.set(dynModule);
      for (const linkKey of linkKeys) {
        module[linkKey].set(dynModule[linkKey]);
      }
    },
  };
}

[jkrems]

@KristianFJones If you want to support named imports[1], you can generate a wrapper module. It's a bit more manual but works very similar:

return {
  type: 'module',
  source: `
import {createRequire} from 'module';

const require = createRequire(new URL(import.meta.url));
const cjs = require(${JSON.stringify(cjsFilePath)});

${exports.map(
  prop => `let $${prop} = cjs[${JSON.stringify(prop)}];`
).join(';\n')}

export {
${exports.map(
  prop => `  $${prop} as ${prop},`
).join('\n')}
}
`,
};

I will throw in that by doing what you do here, you're opting into a different execution order than what people may be expecting: All CJS will execute first and then the ESM modules execute. Which likely won't match how the code ran when everything was compiled to CJS.

[1] "Deconstruct" is usually used when talking about creating new local variables and initializing them with values of object properties or array elements. Named imports expose the original variables and don't create new ones so "deconstruct" is a bit misleading as a term here.

[EntraptaJ]

@jkrems Thanks, is that actual code or just a quick napkin snippet, because I'm unaware of a hook that returns the type and the source. I've been out of the programming scene for 3 months, so I'm a bit behind on the latest development.

[jkrems]

It's something a getSource hook could return. It does require a getFormat hook as well right now to return the format IIRC. It's a napkin snippet but it's pretty close to the actual APIs.

[EntraptaJ]

@jkrems You're a genius. Thanks for your help. I think from this I can get @K-FOSS/TS-ESNode working again! I've gotten it working mostly locally, just need to fix all the other broken things from the last 3 months