fix: fix re-export at esm entry point

[antongolub]

relates (?) #143

[orta]

/cc @weswigham

[weswigham]

First, on this change: This simply changes the import style and doesn't change the file we import, so isn't doing what #143 is asking for at all. I don't really understand why you'd want to do what this PR is doing - more explanation is definitely required.

Second, on #143 itself: the original premise is flawed - we do not want to reexport anything other than exactly what our cjs entrypoint looks at, because we only want a single copy of each of these helpers bouncing around in a runtime (on the offchance any use state, like some template literal helpers were/are). The "newness" of the format of the source code has no bearing on it.

[antongolub]

@weswigham,
I was a bit hasty. I've recognised, that case I'm trying to fix is more exotic: it relates ESM issue in VM module context.

NODE_OPTIONS=--experimental-vm-modules node test/vm-modules/index.js

[weswigham]

You sure it's not just a bug in the node flag? I'm pretty sure it's not supposed to change the behavior of imports.

[antongolub]

I do not consider myself an expert in this, so I cannot say for sure. I only found an example when cjc-to-esm import interoperability seems violated when vm API is used. Perhaps we just should avoid implicit referring to the default export of tslib.js. Anyway, now we have a reproducible bug context and a workaround proposal.

Btw, this case illustrates exactly how Jest loads modules.

CC @SimenB, you may be interested

[antongolub]

@orta, could you run the tests again?

[antongolub]

@weswigham,

Perhaps I'm using vm api incorrectly. Unfortunately, I could not find a way to explicitly specify / inherit the current module load method for vm.module.link calls.



Perhaps you're right about the flag / nodejs bug. The patch, even if it appears, cannot be applied to the past, right? The official doc says, vm.SourceTextModule was introduced in 2018 since Node.js v9.6.0, so if you intend to support vm API for the entire all actual Node.js range [9.6.0..17.1.0], seems reasonable to land this change somehow. Let it be a temporary fix, but it will allow using vm-based mocking for tslib-dependant esm packages right now.





UPD I found another vm API use case in our codebase: to build restricted context for dynamically loaded code in the application server. So it's not just about esm mocking.

[antongolub]

@weswigham, @orta,

is any chance to land this change?

[antongolub]

@weswigham, @orta,

let's merge this:

1. vm-modules compatible
2. nothing is broken in prev behaviour
3. test added

[rbuckton]

I did some local testing and this doesn't actually work. A vm.SourceTextModule describes an ES Module, but tslib.js is a CommonJS module. When you link it you end up with a module with no exports (which is why you get an error about tslib.js having no default export):

import fs from "node:fs"
import { dirname, resolve } from "node:path";
import { fileURLToPath } from "node:url";
import vm from 'node:vm';

const __filename = fileURLToPath(import.meta.url);
const __dirname = dirname(__filename);

const modulerefContents = `
import * as tslib from '../tslib.js';
checkTslib(tslib);
`;
const tslibjsContents = fs.readFileSync(resolve(__dirname, "node_modules/tslib/tslib.js"), {encoding: "utf8"});
const contextifiedObject = vm.createContext({
    checkTslib(tslib) {
        console.log(`typeof tslib: ${typeof tslib}`);
        console.log(`typeof tslib.__assign: ${typeof tslib.__assign}`);
        console.log(`typeof tslib.default: ${typeof tslib.default}`);
        console.log(`typeof tslib.default.__assign: ${typeof tslib.default.__assign}`);
    }
});

async function linker(specifier, referencingModule) {
  if (specifier === '../tslib.js') {
    return new vm.SourceTextModule(tslibjsContents, { context: referencingModule.context });
  }
  throw new Error(`Unable to resolve dependency: ${specifier}`);
}

const m = new vm.SourceTextModule(modulerefContents, { context: contextifiedObject })

await m.link(linker);
await m.evaluate();

The above example (based on your test), prints the following:

typeof tslib: object
typeof tslib.__assign: undefined
typeof tslib.default: undefined
typeof tslib.default.__assign: undefined

Instead, you probably need to use a vm.SyntheticModule for the CommonJS export:

import fs from "node:fs"
import { dirname, resolve } from "node:path";
import { fileURLToPath } from "node:url";
import vm from 'node:vm';
import module from 'node:module';

const __filename = fileURLToPath(import.meta.url);
const __dirname = dirname(__filename);

const modulerefContents = `
import * as tslib from '../tslib.js';
checkTslib(tslib);
`;
const tslibjsContents = fs.readFileSync(resolve(__dirname, "node_modules/tslib/tslib.js"), {encoding: "utf8"});
const contextifiedObject = vm.createContext({
    checkTslib(tslib) {
        console.log(`typeof tslib: ${typeof tslib}`);
        console.log(`typeof tslib.__assign: ${typeof tslib.__assign}`);
        console.log(`typeof tslib.default: ${typeof tslib.default}`);
        console.log(`typeof tslib.default.__assign: ${typeof tslib.default.__assign}`);
    }
});

async function linker(specifier, referencingModule) {
  if (specifier === '../tslib.js') {
    return new vm.SyntheticModule(['default'], function () {
        const wrapper = vm.compileFunction(tslibjsContents, ["module"], { parsingContext: referencingModule.context });
        const module = { exports: {} };
        wrapper(module);
        this.setExport('default', module.exports);
    }, { context: referencingModule.context });
  }
  throw new Error(`Unable to resolve dependency: ${specifier}`);
}

const m = new vm.SourceTextModule(modulerefContents, { context: contextifiedObject })

await m.link(linker);
await m.evaluate();

Which prints:

typeof tslib: object
typeof tslib.__assign: undefined
typeof tslib.default: object
typeof tslib.default.__assign: function

Using vm.SyntheticModule to represent a CommonJS module is the closest behavior to NodeJS's ESM loader behavior, and doesn't require changes to tslib.

[antongolub]

@rbuckton,

Thanks for the explanation. It's clear, we need to refactor our module loader: we expect if some module has an ESM entry point, then all other resources of this package, that are referenced by this point, are also ESM formatted. This hypothesis is wrong, as we now know. Unfortunately,tslib does not use .cjs or .mjs file extensions, which could serve as clues.

[rbuckton]

Leveraging .cjs or .mjs extensions would be problematic on several fronts:

1. We can't rename tslib.js to tslib.cjs as there are too many cases of direct file references to this path in the wild.
2. We also can't rename tslib.es6.js to tslib.es6.mjs for the same reason.
3. We don't want to duplicate the helpers into new files with these extensions as that would result in even more copies of the helpers we'd need to maintain and review.

Also, I'm not entirely sure if there would be any impact for web servers that serve up a copy of tslib to clients, as there's no guarantee that a web server is configured to treat .cjs or .mjs as the text/javascript MIME type.

[antongolub]

That's reasonable, I have nothing to object to.

But it still looks like we need some sort of tweak for esm entry. Here 's tslib export:

    "exports": {
        ".": {
            "module": "./modules/index.js",
            "import": "./tslib.es6.js",
            "default": "./tslib.js"
        },
        "./": "./"
    }

module here explicitly indicates that we load ESM, right? So If we just add "type": "module" to package.json and load tslib/modules/index.js', everything should work fine, nothing is changed. But it fails:

node -e "import('~/projects/gh/tslib/modules/index.js')"
file:///~/projects/gh/tslib/modules/index.js:1
import tslib from '../tslib.js';
       ^^^^^
Uncaught:
SyntaxError: The requested module '../tslib.js' does not provide an export named 'default'

UPD

nvm, seems that my issue is covered by #171