module: disable exports encapsulation for CJS

[guybedford]

Recently, Preact upgraded to using "exports" and hit some compatibility issues with user code like require('preact/package.json'). As a result Preact now has "exports": { "./": "./" } in its package.json, effectively opting out of exports encapsulation entirely to ensure compatibility.

This PR proposes a suggested way to avoid this issue by disabling "exports" encapsulation entirely for CJS require usage.

It's worth noting encapsulation is not a strong encapsulation (URL imports allow getting around it), so that there is no strictness loss here.

I think this approach would be a good compromise to allow adoption without breaking changes in forcing encapsulation for CJS.

//cc @nodejs/modules-active-members

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[hybrist]

@developit Can we get a quick check on this: My understanding is that Preact wasn't really interested in encapsulation. Would this PR (encapsulation would've only affected modules) prevented you from adding ./: ./ to the exports?

My first reaction: I'm a bit concerned that this creates confusing situations. Code that uses Faux Modules via babel on the server would "work" but it would then fail to bundle because webpack implemented exports for modules. And it would create a new natural barrier for switching from Faux Modules to Modules because "everything suddenly breaks". I'd much rather see if we're missing APIs for tools to cleanly access package meta data than to "bless" require to be that API even deep into an import future.

P.S.: That missing API could be an npm package, doesn't need to be in node core.

[GeoffreyBooth]

I thought part of the design of "exports" was that it behaved the same for both CommonJS and ESM. Having encapsulation apply only to ESM seems like quite a change.

I’m also not clear on why this change is needed. The package author doesn’t get encapsulation unless they use "exports"; if they want to both use "exports" but not get encapsulation, they can add "./": "./" like Preact is doing. Why take away encapsulation from all CommonJS "exports" users?

[weswigham]

Very much so - the primary motivation, I thought, was the encapsulation. If a package needs to opt out of that for compat, so be it, but that does in no way mean encapsulation should be tossed out, IMO.

[guybedford]

In case there is any doubt, encapsulation for import is maintained perfectly fine under this PR.

The concern is more that if packages upgrading to exports might want to opt out of encapsulation to provide backwards compatibility (just like how Node.js is locked to continue exporting internal private APIs), that this might provide a better middle ground to ensure encapsulation can continue to work for package authors today.

[hybrist]

this might provide a better middle ground to ensure encapsulation can continue to work for package authors today.

If we only consider node then that's true. But once webpack & friends add support for exports, this becomes a lot less clear. Because they would use the import-variant exports and then there's suddenly a breaking change for existing consumers anyhow. So realistically a package that adopts exports would have to risk breaking some existing users or make it a major change. Even if it only applies to import. I'm not sure excluding require() consumers would fundamentally change this medium-term.

[GeoffreyBooth]

The concern is more that if packages upgrading to exports might want to opt out of encapsulation to provide backwards compatibility ... that this might provide a better middle ground to ensure encapsulation can continue to work for package authors today.

In other words, you want to provide a way for package authors to encapsulate their package for import consumers but not for require consumers. As in, "./": "./" opts out of encapsulation for both, and you wish you could opt out of it just for require. Is that correct?

My concern is that this seems to disable encapsulation for CommonJS; there’s no way to opt in. I think if I had to choose between no encapsulation in CommonJS, versus not being able to opt out of it in one environment separate from the other, I would choose the latter. I guess ideally you wouldn’t have to choose; is there some way to map "./" with conditional exports such that it maps to "./" only for require?

[hybrist]

is there some way to map "./" with conditional exports such that it maps to "./" only for require?

Haven't tested but I would expect the following to work:

{
  "exports": {
    ".": "./main.cjs",
    "./": { "require": "./" }
  }
}

[weswigham]

I think not encapsulating both esm and cjs at the same time exacerbates the issue of a package really being two packages (an esm and a cjs one) jammed into the same namespace, since it causes them to present drastically different "public" APIs. IMO, if you need "backcompat" like this, but still want esm encapsulation, you should probably consider doing a major version bump for esm and cjs encapsulation and breaking your cjs consumers, or shipping a separate package with just encapsulated esm.

[ljharb]

I am strongly opposed to this change; the ENTIRE POINT of exports for me was to apply it to both require and import.

URL imports aren't a thing in node, and filesystem things are a "void your warranty" thing, so this is a strong encapsulation feature to me.

I brought up package.json before; if Preact wants to allow that, they can explicitly add a package.json entry into their "exports" field, just like I've done in all my projects: https://github.com/es-shims/Array.prototype.entries/blob/master/package.json#L19-L20

If this encapsulation is removed, then I think we need to restore the entire ESM flag, and forget about backporting ESM into LTS - it's that important.

[ljharb]

Separately, adding "exports" is intended to be a breaking change in all but the most niche of edge cases, so if Preact did that in "not a major version" then any breakage caused is on them.