Skip to content
This repository has been archived by the owner on Sep 2, 2023. It is now read-only.

Proposal: extension map in package.json #283

Open
ljharb opened this issue Feb 28, 2019 · 30 comments
Open

Proposal: extension map in package.json #283

ljharb opened this issue Feb 28, 2019 · 30 comments

Comments

@ljharb
Copy link
Member

ljharb commented Feb 28, 2019

Internally in the fork, we seem to have an extensionFormatMap, which is something exactly like:

{
  '__proto__': null,
  ".js": "cjs"
  '.cjs': 'cjs',
  '.js': 'esm', // obviously this shouldn't be the default here, but that's a separate discussion
  '.mjs': 'esm'
}

My suggestion is a field in package.json, "extensionsMap". The implementation would roughly be like this:
change https://github.com/nodejs/ecmascript-modules/blob/da0667d4b0c4fcd26b595a5af3fafd6d743cd2d1/lib/internal/modules/esm/default_resolve.js#L75 to:

const packageFormatMap = doMagicToGetThis();
// ^ this would throw if any keys in the object did not match `/^\.[^\.\s]+$/`, say
// ^ this would also either throw, or ignore, if any of the values
//   in the object were not a recognized parse goal
//  (ie, an existing value in `extensionFormatMap`)

const actualFormatMap = { ...extensionFormatMap, ... packageFormatMap };
let format = (legacy ? legacyExtensionFormatMap : actualFormatMap)[ext];

Now, you've got a mechanism to add new extensions, override existing ones, and even set them to null, perhaps, to block them from being loaded - all per package boundary. If this were to land, I'd have no objection to a "type" or "mode" field, as either mutually exclusive sugar for this field, or, as sugar for also merging in { '.js': 'esm' }, or whichever.

Thoughts?

@ljharb
Copy link
Member Author

ljharb commented Feb 28, 2019

cc @GeoffreyBooth

@GeoffreyBooth
Copy link
Member

Thank you for proposing this.

It looks like you proposed this in #160 (comment) and there were objections to using file extensions on both sides of the mapping, because it’s confusing (what does .js represent? Not all users immediately consider it to be CommonJS, and even less so in the new implementation) and because of the potential for circular references. The end result of the discussion in that thread was that the group favored a map of extensions to MIME types.

I am also opposed to requiring users to use any secondary extension, whether .mjs or .cjs, to represent some of the JavaScript files in their codebase. It’s a very common pattern to have ESM .js files in src/ and CommonJS .js files in dist/. Users should be able to continue this practice.

@ljharb
Copy link
Member Author

ljharb commented Feb 28, 2019

It wasn't that "the group favored", it was that some members of the group favored.

What do you mean "secondary extension"? .mjs is the primary ESM extension, full stop.

Either way, you can continue that practice with this proposal by adding a package.json inside src, or dist, or both, that alters the extensions mapping to whatever you like.

@devsnek
Copy link
Member

devsnek commented Feb 28, 2019

@MylesBorins @bmeck there was another thread where myles posted a suggested interface which brad then built upon. I've not been able to find it but if either of you remember what it was i think it would be a good thing to track here as it was similar.

@MylesBorins
Copy link
Contributor

if I recall my proposal was for loaders. the idea would be that we could give canonical names to our built in loaders esm, cjs, native, wasm, json etc. and tie an extnesion to them. although I didn't have the dot in it

An MVP could just be exposing the translators we already have e.g.

https://github.com/nodejs/ecmascript-modules/blob/modules-lkgr/lib/internal/modules/esm/translators.js

which is what the object in https://github.com/nodejs/ecmascript-modules/blob/da0667d4b0c4fcd26b595a5af3fafd6d743cd2d1/lib/internal/modules/esm/default_resolve.js#L18-L23 gets mapped to

so if we were to expose the built in loaders in a @nodejs/ namespace (that's for you @ljharb 😇) I could represent some of our current defaults

--mode=modules

"loaders" {
  "js": "@nodejs/loaders/esm",
  "mjs": "@nodejs/loaders/esm",
  "cjs": "@nodejs/loaders/cjs"
}

--mode=cjs or perhaps --mode=legacy 😇

"loaders" {
  "js": "@nodejs/loaders/cjs",
  "mjs": "@nodejs/loaders/esm",
  "cjs": "@nodejs/loaders/cjs",
  "json": "@nodejs/loaders/cjs",
  "node": "@nodejs/loaders/cjs"
}

but you could also extend this in a bunch of different ways

"loaders" {
  "js": "@standard-things/esm/ng",
  "mjs": "./my-custom-loader.mjs",
  "ts": "typescript-loader-from-npm",
}

We also talked about making it composabe. I just added * as an example below as perhaps a pre processing step for all loader? @nodejs/loaders/reload could be a step that could pipe a module record to another loader based on meta data in the module record.

--mode=modules

"loaders" {
  "*": ["@nodejs/loaders/legacy-extension-resolution", "@nodejs/loaders/reload"],
  "jsx": ["./jsx-2-esm", "@nodejs/loaders/esm"]
}

The above api is all somewhat dependent on brads custom loader implementation, which I have not looked at recently. We would also likely have to refactor our internal loaders quite a bit to allow composability, but we could likely get an MVP together that only support a single loader per extension.

@guybedford
Copy link
Contributor

Just to take a step back here - it would really help to understand what the exact use cases / problems / user stories are that this feature solves.

Is this purely to support TypeScript in Node.js like Deno?

@ljharb
Copy link
Member Author

ljharb commented Feb 28, 2019

@guybedford no, it’s to enable anyone to choose any extensions they want for anything - without that full ability, i don’t think there’s any benefit (in fact i think it’d be harmful) to allow anyone to use anything but the default extensions.

Put another way, this mechanism is the only way (modulo sugar/shortcuts) i think you should be able to have ESM in a .js file.

@guybedford
Copy link
Contributor

no, it’s to enable anyone to choose any extensions they want for anything

Why is this a goal? Who is "anyone" here? I would really appreciate it being phrased in terms of a genuine use case.

@ljharb
Copy link
Member Author

ljharb commented Feb 28, 2019

"Consistency" is a genuine use case - making "ESM in .js" a special, magical, first-class citizen is confusing, and incredibly short-sighted. People have come up with all kinds of use cases for require.extensions that nobody anticipated at the time, and the concept of "loaders" has grown out of that.

Basically, with CJS, users have a default extension for each parse goal, but there's a mechanism to override or augment it - if we're taking that ability away from them for ESM, we're saying "we know better than you do how extensions should work" - and in that case, why not force .mjs for ESM for everyone?

If, instead, we want to say "you, the user, have the ability to use extensions as you see fit, but we're providing defaults", then every extension and parse goal should be equally customizable.

@guybedford
Copy link
Contributor

if we're taking that ability away from them for ESM, we're saying "we know better than you do how extensions should work" - and in that case, why not force .mjs for ESM for everyone?

We're not taking this ability away. The resolver can be intercepted with custom behaviours just like require.extensions supports. And actually a custom resolve is a far more general way than require.extensions supports.

If, instead, we want to say "you, the user, have the ability to use extensions as you see fit, but we're providing defaults", then every extension and parse goal should be equally customizable

This is exactly what we're doing. Users can override the resolver if they want to and inject this customization.

I guess my primary concern is that when something is in the package.json that means it is now being published to npm with those customizations, and I can install an npm package that I don't necessarily know is making these customizations. Now say I want my http server to serve to the browser, but it won't know what MIME type to use for the module that is now stored at file.module that my server doesn't understand but Node.js has understood without telling me because we added a deep customization to the pacakge.json that was invisible to me the user.

Similarly, as mentioned many times before, I'm very concerned about users shipping custom languages to npm uncompiled. This creates a performance and tooling problem that is "invisible" if everything just worked after npm install pkg && node -e "import('pkg')". Only after many packages installed do I then realise I'm compiling 10000 lines of TypeScript on every Node execution.

I guess I just want to understand the user use cases that drive this need, so we can do this in a way that doesn't become a viral customization that will start to impinge on the health of the ecosystem.

@devsnek
Copy link
Member

devsnek commented Feb 28, 2019

Now say I want my http server to serve to the browser

you need to use a bundler for this case because of cjs and node-specific apis and etc. the idea of node_modules somehow being magically browser safe is absolutely untenable.

I guess I just want to understand the user use cases that drive this need, so we can do this in a way that doesn't become a viral customization that will start to impinge on the health of the ecosystem.

maybe we can use gzemnid to find packages that modify require.extensions. i think the goal here is to provide better scoping so one package doesn't break another, which can happen with the current design.

another part is that the current --type api is pretty terrible. this is a pretty good alternative imo.

@ljharb
Copy link
Member Author

ljharb commented Feb 28, 2019

@guybedford that same thing applies to a "type" or "mode" or "exports" field in package.json - you always have to know the possibilities, and build your tooling to accomodate them - otherwise, we can't allow any deviation from the defaults (which includes, writing ESM in anything that doesn't end in .mjs).

@guybedford
Copy link
Contributor

@guybedford that same thing applies to a "type" or "mode" or "exports" field in package.json - you always have to know the possibilities, and build your tooling to accomodate them - otherwise, we can't allow any deviation from the defaults (which includes, writing ESM in anything that doesn't end in .mjs).

Certainly, but "type": "module" in package.json does not create a pattern for publishing packages on npm that will lead to me unwittingly installing code that compiles at runtime.

@ljharb
Copy link
Member Author

ljharb commented Feb 28, 2019

I'm not sure what you mean by "compiles at runtime"; there's nothing about this proposal that forces that, and as for "generating the import map", that's also something npm could optimize by doing at publish time.

@guybedford
Copy link
Contributor

I'm not sure what you mean by "compiles at runtime"; there's nothing about this proposal that forces that,

Ahh, sorry I was referring to @MylesBorins's proposal here.

Just to clarify, your proposal is the following:

package.json

{
  "extensionMap": {
    ".asdf": "esm"
  }
}

where the possible keys are any extension and the possible values are the "translators" in core, which is currently just "esm" and "commonjs".

My concern is specifically when we allow packages to define their own "translators" easily.

@ljharb
Copy link
Member Author

ljharb commented Feb 28, 2019

@guybedford yes, exactly. Per-package loaders/translators is not what i am proposing and in no way do i think that's necessary for my use cases.

@weswigham
Copy link
Contributor

My concern is specifically when we allow packages to define their own "translators" easily.

Even with no first class support, you could write an esm package that compiles some code at startup and then imports it using dynamic import. Not really sure how that's any better or worse.

so if we were to expose the built in loaders in a @nodejs/ namespace (that's for you @ljharb 😇) I could represent some of our current defaults

I would love if the builtin mechanisms for accomplishing default behaviors were exposed like this (assuming those paths actually resolve in normal code to a real thing-that's-used-as-part-of-resolution). That makes it much easier to 1. track upstream changes, and 2. compose them. Months ago, that's what I was requesting we do for the named-exports-for-builtin-libraries-loader; if it's so useful it's used within node, I see no reason why people using node shouldn't be able to choose to use it, should their constraints align similarly.

@guybedford
Copy link
Contributor

Even with no first class support, you could write an esm package that compiles some code at startup and then imports it using dynamic import. Not really sure how that's any better or worse.

There are issues with this for dynamic import due to the fact that transitive rewriting into data / blobs gets into complications with circular references. This requires deep technical knowledge to pull off properly, while a package.json hook can be used by anyone. Which is a huge difference in adoption.

I would love if the builtin mechanisms for accomplishing default behaviors were exposed like this (assuming those paths actually resolve in normal code to a real thing-that's-used-as-part-of-resolution). That makes it much easier to 1. track upstream changes, and 2. compose them. Months ago, that's what I was requesting we do for the named-exports-for-builtin-libraries-loader; if it's so useful it's used within node, I see no reason why people using node shouldn't be able to choose to use it, should their constraints align similarly

As mentioned in #283 (comment), it appears this is not what @ljharb is proposing or asking for in this issue here. We are just considering the extensionMap proposal as above. Any feature or extension of per-package loaders should be a phase 3 concern and discussion we shouldn't go into now.

@ljharb
Copy link
Member Author

ljharb commented Mar 1, 2019

ok, so, to recap:

  1. This is not proposing per-package loaders nor exposing loaders nor not exposing/having them
  2. the justifications for this to exist are virtually identical to the justifications for being able to write ESM in .js files
  3. there is a possibility that some members of the group want to use mimes instead of extensions - this is a fine bikeshed to have after we can agree that this approach is solid and wanted
  4. this proposal does not obstruct any other use cases nor make them any more dynamic than they already are (assuming we already have a package.json field to allow ESM in a file that doesn’t end in .mjs)
  5. If this proposal is rejected (the concept, not the specifics of the implementation) then i personally see that as tantamount to requiring ESM only ever be written in .mjs or extensionless files, and in that case that’s the direction we should proceed.

What objections have i missed? Are there any others?

@mcollina
Copy link
Member

I would like to note that Node.js does not have a great support for source maps. Some diagnostics tools do not even make sense in a source-mapped world (flamegraph for example). The moment this gets widespread we are losing some of the diagnostics capabilities of Node.js: "hidden transpilation" is not something I would recommend to run in any production server. I know other disagree, and that's ok.

The toolchain should include a mode to "eject" and have it all working without "hidden transpilation".

@guybedford
Copy link
Contributor

@mcollina on that note I've always wondered why Node.js doesn't support source maps natively? http://npmjs.org/package/source-map-support is incredibly widely used and seems a straightforward approach to adopt natively. Would be interested to hear the arguments.

Hidden transpilation is a huge worry though, yes, for many reasons.

@mcollina
Copy link
Member

@mcollina on that note I've always wondered why Node.js doesn't support source maps natively? http://npmjs.org/package/source-map-support is incredibly widely used and seems a straightforward approach to adopt natively. Would be interested to hear the arguments.

I think that's a different conversation. Probably better to open it in nodejs/node. It's also something that we could change.

@ljharb
Copy link
Member Author

ljharb commented Mar 16, 2019

@mcollina hidden transpilation is not relevant to this issue whatsoever; I’m not proposing it, and it’s already possible today and people do it anyways.

@mcollina
Copy link
Member

I know people are doing it. However, most module author ships transpiled version when publishing to npm.

This is proposing hidden transpilation on a per module-basis, as a core-recognized way of doing things. This is the principle I’m in disagreement.

I think this would be great to have if some sort of ejection mechanisms is taken into consideration when designing things.

@ljharb
Copy link
Member Author

ljharb commented Mar 16, 2019

@mcollina it is most certainly not proposing that - that would be loaders.

All I’m proposing is static config in package.json to remap already supported extensions and parse goals.

@mcollina
Copy link
Member

Oh, I misread some of the thread. I'm definitely in favor of your proposal.

@ljharb
Copy link
Member Author

ljharb commented Apr 8, 2019

Here’s a concrete user use case that this proposed generic solution would address: nodejs/help#1850

@SMotaal
Copy link

SMotaal commented Apr 9, 2019

@ljharb do you want to put this on the agenda for this week? Elaborating on the use case can only help.

@ljharb
Copy link
Member Author

ljharb commented Apr 9, 2019

Myles and I haven’t had a chance to discuss it yet; I’m not sure it’s worth agenda time prior to that.

@bmeck
Copy link
Member

bmeck commented Nov 19, 2019

bump in light of this seeming to be relevant to nodejs/node#30514

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants