Naming bikeshed - require(esm) interop flag

[guybedford]

As discussed in today's meeting, we need to come up with a name for the flag that will allow require(esm) to support a custom value for a CJS module.

The pattern is that users can write:

export const __cjsModule = true;
export default module.exports;

where module.exports can be any type of object, which will be returned by require(esm) when it sees this __cjsModule flag, instead of returning the full namespace.

@joyeecheung suggested a good name might be one that describes what it does, eg cjsUnwrapDefault.

It should be unique enough that it will not collide with an existing export name for any ES module in use today.

Opening this thread for further discussion and suggestions, although I'm happy to go with something like @joyeecheung's option here above as well.

[mcollina]

I would still prefix it with __, so __cjsUnwrapDefault would be good for me.

[guybedford]

@mcollina thanks, I do tend to support similar, would you be able to share motivation further here for the prefixing?

[mcollina]

Let users seeing this it's something internal.

[guybedford]

In the case of libraries shipping ES modules that they want to define with non-object values in CJS, the goal is for authors to add this themselves. So perhaps that’s an argument against it then?

…

On Sun, Aug 25, 2024 at 19:55 Matteo Collina ***@***.***> wrote: Let users seeing this it's something internal. — Reply to this email directly, view it on GitHub <#221 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAESFSRKXL3L5VACLLESNIDZTKKJJAVCNFSM6AAAAABMPELMI2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBZGIYDCMZVGU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[joyeecheung]

I am not sure how internal this actually is - it's meant for library authors who want to ship ESM to keep their exports look the same to CJS users. Say for this pretty common pattern on npm:

// package foo is just this file
module.exports = function foo () { ... };

Now the author wants to ship it as ESM, then they change it to

// package foo is just this file
export default function foo() { ... }

Now that works for ESM users, but then for CJS users, it would require code changes for them to use the newer version of the library (shipping ESM-only should better be semver-major regardless, because it makes the exports un-mockable, since ESM exports are immutable - but at the end of the day this is up to the discretion of the library author).

const foo = require('foo');  // This now doesn't work
const foo = require('foo').default;  // It now needs to be like this

So the new flag helps library authors to unwrap their default exports in require(), which means they should write the ESM version as

export default function foo() { ... }
export const whateverThisFlagIsCalled = true;  // This tells Node.js to return namespace.default from require()

Now to their existing CJS users, require('foo') just returns default exports without further unwrapping. This creates a disparity between require() and import() - obviously to support something similar in the latter one needs to convince TC39 for some special syntax, but at least in the meanwhile we have some kind of polyfill to help libraries targeting Node.js to transition. In any case, this is likely to be hand-written by library authors, so it's not very internal IMO. The only internal part is to their ESM consumers, as they will see it via import * as mod from 'foo' or import('foo'), but then it's going to be a well-documented flag, and it may be useful to them if they actually want to test around it, so I am not sure what we get from trying to hide them.

[guybedford]

Okay, agreed that as a user feature and since we have a very unique name, that removing the prefix can be considered. I'm updating the PR to that effect now.

[ljharb]

I dunno, it's still a magic string, and __esModule is already a thing - __something seems like a more consistent and safe choice.

[marco-ippolito]

I dunno, it's still a magic string, and __esModule is already a thing - __something seems like a more consistent and safe choice.

I agree it should start with __ so its pretty obvious its an internal (or meta)

[joyeecheung]

__esModule is a convention used by tools and humans are not supposed to hand-write them. But this flag is meant to be hand-written by human beings.

I don’t feel strong enough to insist on it, anyway. Just be aware this flag isn’t private - it will be documented, and is meant to be hand-written by human beings.

[nicolo-ribaudo]

If this was not on the module namespace object but on a random object, it would probably be a symbol. It's not to hide it, but just to clearly communicate that it's a special value and not just part of the arbitrary keys namespace that random objects can have.

For example, we have Symbol.for('nodejs.util.inspect.custom') and we don't just ready a random utilInspectCustom property on objects, even if that symbol is for hand-written custom inspection implementations that libraries can provide for their objects.

We cannot have symbols on module namespaces, but we should still have some visual marker that says "this property is somewhat different from the other exports".

[joyeecheung]

I wonder if we can do this instead:

function foo() {}

export { foo as "module.exports" };  // Or prefix it with `nodejs`, but "module.exports" feel way nicer
export default foo;

[mcollina]

@joyeecheung love it

[ljharb]

That's already a valid export name, though (as are all strings), so I don't think it avoids the need for a human-visible string indicator that it's internal to node, like __.

[joyeecheung]

These are all valid names, but "module.exports" is probably special and Node.js specific enough (that and string names tend to be rarer than normal names already). Also I think the intention of this is the easiest to guess.

[nicolo-ribaudo]

When I said "it should be have a marker that shows that it's not a normal export" what I was only thinking about a prefix, like __ or @@. I refrained from suggesting @@ (which usually represents a symbol-like string) because it would have required two lines to be written:

export default function foo() {}

let unwrapDefault = true;
export { unwrapDefault  as "@@unwrapDefault" };

However, I quite like the module.exports suggestion:

• it's not namespaced with a prefix, but it has a . in the middle making it basically impossible to accidentally conflict with some export name that somebody might want to use
• it doesn't require one more line than an export const __unwrapDefault = true;, as I can do export default function foo() {} and then export { foo as "module.exports" } or export { default as "module.exports" } from "this-file"
• for developers that know CommonJS, it's incredibly clear what it means. There is very little risk of confusion, and it's a very easy-to-remember name.

I would have a question though: if we are exporting the module.exports value and not just a boolean, what happens if export default and export { ... as "module.export" } don't match? We could either error, or allow it. Erroring would catch more mistakes, but allowing it would make it possible to write code like

export const PI = 2.14;
export default function circ(radius) {
  return PI * radius * 2;
}

// for CJS, we still want to have a "default" function export and a "named" variable
const circClone = circ.bind();
circClone.PI = PI;
export { circClone as "module.exports" }

[joyeecheung]

We could either error, or allow it. Erroring would catch more mistakes, but allowing it would make it possible to write code like

I think it's always better to give more options to the users, and let linters catch these mistakes instead of closing the doors with a runtime error.

[guybedford]

I like the sentiment of module.exports here, but one of the goals of this work is to align this interop with the wrappers produced when requiring a CJS module from ESM (via import('cjs')), so that they will also reflect this marker. In that case having the standard representation of CJS in ESM have this shape of Namespace { 'module.exports', default, ...detectedNames }might introduce more confusion for consumers of CJS modules who aren't even aware of this interop history in future in terms of which import to use, and especially when combined with the fact that users would use this feature for tricks where the default export and module.exports values may or may not then match it may result in some unintended consequences.

We've already incurred the cost of the conceptual simplicity of the default export exactly representing CJS module.exports for ESM wrappers, as it was designed for in TC39. This interop problem for defaults and named exports cases was already the difficult tradeoff we had to make and is part of the Node.js compat model now. If we hadn't yet made this decision I'd be more sympathetic here to models where module.exports captures the CJS value and default could be flexible (and Joyee it would have been so great if you'd been around for those discussions, this would have been an interesting path!). But unfortunately, we just can't entertain those kinds of models at this point due to compat.

Given we have the default export model for CJS representation in ESM, the private marker remains my preference. And the __ prefix was also my original choice. The original consensus in this bikeshed discussion was to remove it, but since others chimed in the consensus shifted to include it. Therefore I've added back this prefix in the PR at nodejs/node#54563.

[joyeecheung]

I propose that we do a poll either within TSC or among the bigger collaborator/user base. WDYT?

[joyeecheung]

In that case having the standard representation of CJS in ESM have this shape of Namespace { 'module.exports', default, ...detectedNames } might introduce more confusion for consumers of CJS modules who aren't even aware of this interop history in future in terms of which import to use

This assumes that we actually want to mutate the CJS wrapper namespace, which I don't think we have consensus about anyway. If { 'module.exports', default, ...detectedNames } can look confusing to those who import(cjs) in the future, I am not sure why { __cjsUnwrapDefault, default, ...detectedNames } can look less confusing. I would just suggest for the purpose of detecting imported CJS, we go with the alternative suggested in nodejs/node#53848 (comment)

import { isCJSModule } from 'node:module';  // Or use process.getBuiltinModule();
import * as empty from './empty.cjs';
isCJSModule(empty);  // true

// Instead of checking empty.whateverFlag that is added by Node.js to CJS wrappers

I think that would be a less confusing and more generally useful API overall. So we get two more intuitive APIs, instead of introducing a cryptic API that tries to do both.

[ljharb]

Would there be a similar predicate for isESMModule?

Either way wouldn’t those predicates cause changing format to be a breaking change? (which it is now, but hopefully require(esm) allows it to no longer be one)

[guybedford]

This assumes that we actually want to mutate the CJS wrapper namespace, which I don't think we have consensus about anyway

There are two confusions I was pointing out, and I'll repeat the arguments so these are clear:

1. As an ESM importer importing CJS, what if I start relying on the module.exports instead of default export name? This is a choice for users to make, and perhaps some patterns will emerge for one over the other. The confusion specifically being, which to use.
2. As an ESM importer importing ESM that was designed to be consumed by CJS, I will also get this module.exports property, which in that case may or may not also come with a default export. The confusion here being that module.exports and default diverge, but as the consumer I don't necessarily know if I'm importing CJS or ESM so this introduces semantic ambiguity to me as the consumer of the module as to the expectation when importing a module.

Using the default aligns the consumer expectation on a default import in both cases, removing ambiguity. And where interop is a complex enough space, removing ambiguity helps build alignment between libraries and tools.

@joyeecheung this is a naming bikeshed discussion, not a debate over isCJSModule. I've engaged fully on these issues. I think it would be best if you make your intent explicit - either request a block on nodejs/node#54563 landing or not and we can then continue process from there.

[joyeecheung]

I find those questions are a bit far-fetched. How often do people import * from a package and decide to what export to use based on personal guessing about the name, without consulting the documentation? I feel that in most cases, you just follow (copy-paste) what the documentation tells you to use. If there are any undocumented exports, a regular user is not even likely to notice them, or would not think about using them unless they know what they are doing.

[nicolo-ribaudo]

If there are any undocumented exports, a regular user is not even likely to notice them

This is unfortunately not true, as editor autocompletion shows all the available exports.

[joyeecheung]

Using the import syntax, how do you actually get the string name autocompleted though? At least when I try with VSCode, it doesn't work with string names. It's also another story about how the import ... from ... syntax itself doesn't work with autocomplete that well in the first place - personally I have rarely used it with import syntax because of this, and when I do I already know what export I am looking for (otherwise I don't know what to type after import). When I am using a library I don't know well, I just copy paste the import statement from the documentation.

We could also just suggest editors not to autocomplete that specific name (if they do support it), or tools to not emit definitions for this (I guess the same needs to be done for __cjsUnwrapDefault anyway). At the end of the day, common sense applies - it's an undocumented export if the package author doesn't document it, and people typically just ignore undocumented stuff. Is this concern really worth asking the package authors to hand-write a more cryptic export?

[nicolo-ribaudo]

Using the import syntax, how do you actually get the string name autocompleted though? At least when I try with VSCode, it doesn't work with string names.

You need to use TypeScript 5.6 beta, which is the only TS version that supports export { x as "y" } and it's not released in VSCode yet. Given this file:

let a;
export { a };
export { a as "x" };
export { a as "module.exports" };

I get these suggestions:

It is however possible to hide it, even if it's opt-in:

let a;
export { a };
export { a as "x" };
export { /** @deprecated */ a as "module.exports" };

[aduh95]

My not so informed preference would be:

1. Always unwrap default export if it's the only export (although I've been told that might be too "magical", but IMO it's always going to feel magical no matter what we decide, and I think that's actually the most straight forward thing from a user perspective).
2. export { myDefaultExport as "module.exports" } – the rule of thumb being "if you re-defined module.exports in CJS, you must export it as such when refactoring to ESM"
3. export const __cjsUnwrapDefault = true
4. export const cjsUnwrapDefault = true

[joyeecheung]

Always unwrap default export if it's the only export

This would make adding named exports to the ESM semver-major, though. Also previously a CommonJS package can do something like this to provide both default and named exports to their CJS + ESM users:

class Klass { static foo() {} };
module.exports = Klass;
module.exports.foo = Klass.foo;  // make the lexer detect this

If only the single default exports can be unwrapped for CJS users, they need to choose between either breaking CJS users for keeping named exports, or breaking ESM users for losing the named exports. That would demotivate migration to ESM in the first place or keep more people clinging on to faux/dual packages. But if we want to add a marker to help with the case where both default and named exports, then it would be less of a footgun to require it always when there is a default export/it used to reassign module.exports.

[jsumners-nr]

This is not the thread to discuss the which module systems to keep or drop from Node.js.

[joyeecheung]

Marked the off-topic comments as off-topic. @ert78gb if you want to propose breaking CommonJS, please open a separate issue.

[guybedford]

The vote concluded today in favour of module.exports in nodejs/TSC#1622 per #221 (comment). Will update the relevant PRs in this direction.