Naming bikeshed - require(esm) interop flag

guybedford commented 1 month ago

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 commented 1 month ago

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

guybedford commented 3 weeks ago

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

mcollina commented 3 weeks ago

Let users seeing this it's something internal.

guybedford commented 3 weeks ago

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 https://github.com/nodejs/loaders/issues/221#issuecomment-2309201355, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAESFSRKXL3L5VACLLESNIDZTKKJJAVCNFSM6AAAAABMPELMI2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBZGIYDCMZVGU . You are receiving this because you authored the thread.Message ID: @.***>

joyeecheung commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

__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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

@joyeecheung love it

ljharb commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 https://github.com/nodejs/node/pull/54563.

joyeecheung commented 1 week ago

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

joyeecheung commented 1 week ago

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 https://github.com/nodejs/node/pull/53848#issuecomment-2244500584

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 commented 1 week ago

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 commented 1 week ago

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:

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.
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 https://github.com/nodejs/node/pull/54563 landing or not and we can then continue process from there.

joyeecheung commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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" };

ert78gb commented 5 days ago

I am following this thread for a while and hesitated to share my thoughts.

Honestly, I am really curious about the future of the commonjs. If it will part of the node.js ecosystem forever then make sense to improve it and align with es6 module. But if commonjs will remove in the next 2 LTS version I think make no sense to put any effort on it.

I don't think node.js need to support 2 module system in long term. As far as I know commonjs is not part of the EcmaScript standard so it could be removed because it will not break the web.

Remove commonjs will a pain on short term but I don't think it will bigger pain than maintaining 2 module system. Longterm the Node.js core team and library maintainers will win from the retirement of the commonjs

Sorry if I hurt someone with this thought, but sometimes have to sacrifice things for a bigger one.

joyeecheung commented 5 days ago

Personally I am not a believer of paternalistic breaking changes - we should set the breaking timeline based on how much the usage of CommonJS has dropped in the ecosystem instead of a paternalistic"X versions later". If the ecosystem is still clinging on to CommonJS (see https://github.com/wooorm/npm-esm-vs-cjs/ for some data), then I think what we need to do is to provide enough motivation and a good enough migration path for people to move on, otherwise what's more likely to happen is that many will just get stuck with the older releases forever or simply fork the project to keep their code working. That's why we are making require(esm) possible so that package authors can ship native ESM without hurting the popularity of their packages, and support their users to use ESM in plugins/tests/configs...etc. so that more people can move on.

aduh95 commented 5 days ago

My not so informed preference would be:

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).
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"
export const __cjsUnwrapDefault = true
export const cjsUnwrapDefault = true

joyeecheung commented 5 days ago

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.

ert78gb commented 5 days ago

Personally I am not a believer of paternalistic breaking changes - we should set the breaking timeline based on how much the usage of CommonJS has dropped in the ecosystem instead of a paternalistic"X versions later".

Sometimes, have to kindly push people to the right direction.

The https://github.com/wooorm/npm-esm-vs-cjs/ is really nice chart, but if I want to show the esm has bigger part of the bar then I can find the way of it. Just an example the top 25 package from the https://github.com/wooorm/npm-esm-vs-cjs/blob/main/data/2024-08-28.json

esm => 12 (48%)
dual => 6 (24%)
faux => 0 (0%)
cjs => 7 (28%)

If I say dual modules already made the esm transitions then I can say 72% vs 28% the winner is esm. This is why I believe only in statistics that I made (faked). :) The last 25 packages are all cjs.

Honestly, I don't know what is the good measurement. Maybe worth to check the trend of the new packages too.

There is nothing to force the commonjs transition to esm because commonjs works with both environment. But people started it because it has benefit. It is the official js module that supported by browser, better treeshaking, etc. The maintainers of commonjs packages or applications start the feel the pain when they would like to use a pure esm package and based on top of the top high-impact npm packages the chance higher day by day. If you deliver the require(esm) feature then you eliminate problem so the commonjs will with us forever.

I don't wanna force anyone to do anything but please consider how will affect the require(esm) feature to the whole node.js ecosystem. I as a developer who use node.js 60 hours/week feel the pain of some earlier made decision that was full with good will, but it was not fully finished or bought up new requirement/problems that has not been solved in the last 2.5 years.

n.b. The import function is and was the workaround in the last 5 years in the commonjs packages. Not the best solution but it worked and helped in the transition for me.

ljharb commented 5 days ago

Nobody needs a push for anything. If the incentive - the pull - isn't sufficient, then the new thing simply isn't better.

jsumners-nr commented 5 days ago

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

joyeecheung commented 5 days ago

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

nodejs / loaders

Naming bikeshed - require(esm) interop flag #221