Static and dynamic imports inside loader code should have same behaviour

davidje13 commented 2 years ago

I have been playing around with the loaders API in NodeJS and found an inconsistency which caused me some problems:

If a loader imports another module using a static import, that import will not be processed through the loader (this wouldn't be possible, so is expected behaviour). However, if the loader uses a dynamic import, that import will be processed by the loader (even if the same import was previously referenced statically).

Example

index.mjs

#!/usr/bin/env node --experimental-loader ./loader.mjs

import a from './imported.mjs';
console.log(a);

imported.mjs

export default '<imported: preprocessing-result-here>';

loader-imported.mjs

export default '<loader-imported: not processed>';

loader.mjs

import { readFile } from 'fs/promises';

import staticImport from './loader-imported.mjs';

export async function load(url, context, defaultLoad) {
  if (url.includes('loader-imported.mjs')) {
    const raw = await readFile(new URL(url).pathname, 'utf-8');
    return { format: 'module', source: raw.replace(/not processed/g, 'processed') };
  }
  if (url.includes('imported.mjs')) {
    const { default: dynamicImport } = await import ('./loader-imported.mjs');
    const raw = await readFile(new URL(url).pathname, 'utf-8');
    return { format: 'module', source: raw.replace(/preprocessing-result-here/g, 'processed with static: ' + staticImport + ', dynamic: ' + dynamicImport) };
  }
  return defaultLoad(url, context, defaultLoad);
}

Result

<imported: processed with static: <loader-imported: not processed>, dynamic: <loader-imported: processed>>

Expectation

For my use-case, I would prefer if loaders were considered entirely separate; never operating on their own imports (regardless of whether they are static or dynamic), i.e.:

<imported: processed with static: <loader-imported: not processed>, dynamic: <loader-imported: not processed>>

For more general context: I am using loaders to apply customisable pre-processing stages during a test run. So for example, if the user wishes to run the code through Babel before executing, the loader will dynamically import Babel and apply it. But they might choose to use TSC instead, or something else, so dynamic imports are used to avoid hard dependencies on tools the user isn't using. Another possible approach would be to have entirely separate loaders per tool but that gets a bit messy.

guybedford commented 2 years ago

This is definitely a bug - loaders are supposed to have their own un-instrumented loader which is not affected by loaders at all.

@davidje13 did you try checking other Node.js versions to see if this originated at some point. A great start might be to bisect this one as I'm pretty sure it was not always the case.

davidje13 commented 2 years ago

I don't know how to get this running in Node 11 or below (--experimental-loader isn't a recognised flag, and --loader seems to be ignored), but I can see this issue existed at least as far back as 12.17.0: https://gitlab.com/davidje13/nodejs-loader-test/-/jobs/2519108656, which is the oldest version I can get to use the loader at all

(13.2.0 also fails to apply the loader, presumably unrelated)

guybedford commented 2 years ago

Ok thanks, it may well be that dynamic import has never fully taken into account the loader context. Shadow realm makes this process more explicit, hopefully we can take hints from that with regards to implementation.

nodejs / node