Possible loader chaining problem / possible `yarn` problem

[izaakschroeder]

Version

v20.3.1

Platform

Darwin Kernel Version 22.5.0: Mon Apr 24 20:52:24 PDT 2023; root:xnu-8796.121.2~5/RELEASE_ARM64_T6000 arm64

Subsystem

esm, loaders

What steps will reproduce the bug?

• Have Yarn 3+ installed
  • corepack enable
  • yarn set version berry
  • yarn set version 4
• Write a custom loader that uses the load hook
  • Within that hook use await import(someFile);
  • Within someFile import a package that requires another loader to resolve

Reproducible repo: https://github.com/izaakschroeder/loader-chain-issue

Run the following:

yarn
cd packages/demo
yarn demo

---

/packages/banana-loader/loader.mjs:

import * as path from 'node:path';
import module from 'node:module';
import { fileURLToPath } from 'node:url'


const DEFAULT_EXTENSIONS = ['.banana'];

const compile = async (code, filepath) => {
  // We are second loader in chain so we have PnP injected yay all this works
  const pnpApi = module.findPnpApi(filepath);
  const packageLocator = pnpApi.findPackageLocator(filepath);
  const packageInformation = pnpApi.getPackageInformation(packageLocator);
  const configFile = path.join(packageInformation.packageLocation, 'banana.config.mjs');

  // This fails complaining that "banana-config" cannot be resolved
  // This confuses me because shouldn't PnP be active at this point? If the previous
  // code works then how can this not? And specifically importing `configFile`
  // itself is OK, only the import WITHIN it fails.
  const configModule = await import(configFile);

  if (configModule.default.magic !== 42) {
    throw new Error('Bad banana');
  }

  return code;
}

export const load = async (url, context, nextLoad) => {
  if (DEFAULT_EXTENSIONS.some((ext) => url.endsWith(ext))) {
    const {source, responseURL, format} = await nextLoad(url, {...context, format: 'module'});    
    const code = await compile(source.toString('utf8'), fileURLToPath(responseURL));
    return {
      format,
      source: code,
      shortCircuit: true,
    };
  }
  return nextLoad(url, context);
}

/packages/demo/banana.config.mjs:

import {createConfig} from "banana-config";

export default createConfig();

/packages/demo/test.banana:

console.log('Hello world');

/packages/banana-config/config.mjs:

export const createConfig = () => {
  return {magic: 42};
};

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior? Why is that the expected behavior?

Using await import(...) should respect the existing loader chain.

What do you see instead?

Module resolution fails inside an import'd module.

Additional information

No response

[aduh95]

I think that's a known limitation, static imports will use only the previous loaders, dynamic imports will use whatever loaders are available at the time of the import. Calling import() inside a hook will create infinite recursion and should be avoided.

[izaakschroeder]

Interesting 🤔 I'd love to learn a bit more. Would it be possible to point out the area in the code that is responsible for this behaviour? My mental model is currently something like this:

const loaderStack = [];

const import = async (file) => {
  let resolved = null;
  for (loader of loaderStack) {
    // if this calls import() we have a loop
    // UNLESS `loaderStack` is somehow twiddled with each time
    resolved = await loader.resolve();
    if (resolved.shortCircuit) { break; }
  }
  let result = {source: await fs.loadFile(resolved) };
  for (loader of loaderStack) {
    // if this calls import() we have a loop
    // UNLESS `loaderStack` is somehow twiddled with each time
    result = await loader.load(result);
    if (resolved.shortCircuit) { break; }
  }
  return result;
}

const addLoader = (file) => {
  loaderStack.push(await import(file));
}

[aduh95]

https://github.com/nodejs/node/blob/a40a6c890afe0d1d0ca78db015146178c25af079/lib/internal/modules/esm/utils.js#L152-L163

The mental model you presented does not correspond to the loader API (the next hook is available as an argument, and the order is different), but I'd say it's good enough to understand what's happening: once the loader module is loaded, a dynamic import would be loaded by a loaderStack that has grown since the static imports have been loaded.

[izaakschroeder]

Ah excellent thanks @aduh95 😄 I'll have a look in that file!

[izaakschroeder]

@aduh95 thanks for pointing me in the right direction 😄 I'm not sure if this approach is sensible or not but I have a working patch now that solves the problem: https://github.com/nodejs/node/pull/48559 Running it against https://github.com/izaakschroeder/loader-chain-issue now produces correct output!

NODE=/PATH/TO/nodejs/node/out/Release/node

"${NODE}" \
  --require ../../.pnp.cjs \
  --loader ../../.pnp.loader.mjs \
  --loader banana-loader \
  ./demo.banana

Outputs:

Hello world