Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

esm: add package type wasm #31388

Closed
wants to merge 3 commits into from
Closed

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Jan 16, 2020

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

The main benefit of this is to allow files without an extension to be treated as WASM. This does alter the logic for detecting if something should be treated as going through the ESM loader for the main module slightly by inverting the condition from checking for "module" to instead check it doesn't match "commonjs"

This remains flagged behind the WASM modules flag

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. labels Jan 16, 2020
@bmeck bmeck added the modules-agenda Issues and PRs to discuss during the meetings of the Modules team. label Jan 16, 2020
@devsnek
Copy link
Member

devsnek commented Jan 16, 2020

wasm is already a "module". i don't think this change is quite right.

@bmeck
Copy link
Member Author

bmeck commented Jan 16, 2020

@devsnek package types are format mappings, this just adds another set of format mappings to allow files without extensions to be seen as WASM.

@devsnek
Copy link
Member

devsnek commented Jan 16, 2020

package types are format mappings

interesting. i guess we should change the logic to just pass whatever value is present straight through to the resolver unless it equals "commonjs" right? (and maybe rename "module")

@bmeck
Copy link
Member Author

bmeck commented Jan 16, 2020

@devsnek I think that passing things around to the resolver would be a different API/PR. People have already asked about a getPackageType function in other issues and that might be simpler.

@jkrems
Copy link
Contributor

jkrems commented Jan 16, 2020

What I dislike about this change is that type previously had a relatively simple meaning: It determined the file format of both .js files and files without extension:

  • module meant "interpret both .js and no extension as a JS module".
  • commonjs meant "interpret both .js and no extension as a CJS file".

After this lands, there's now a gray area: wasm means "interpret no extension as WASM" but .js is left ambiguous. Without reading too closely I assume it interprets .js as... a JS module? But it's certainly not obvious to me. Maybe an object syntax would be more helpful at that point?

{
  "type": {
     "ambiguous javascript": ": "module",
     "extension-less": "wasm"
  }
}

But that opens a whole can of worms about configuring all kinds of file extension mappings. I'm pretty close to saying "do we need to support extension-less wasm"?

@devsnek
Copy link
Member

devsnek commented Jan 16, 2020

This makes me again question the package.json field additions :(

@bmeck
Copy link
Member Author

bmeck commented Jan 16, 2020

@jkrems how is ".js" ambiguous? It has a specified value (which is "module" in this case as you guessed)

But that opens a whole can of worms about configuring all kinds of file extension mappings. I'm pretty close to saying "do we need to support extension-less wasm"?

The same could have been said about extension-less ESM. I'm not sure how wanting to run a purely WASM application would be considered a bad thing given WASI and the like coming in the future.

@jkrems
Copy link
Contributor

jkrems commented Jan 16, 2020

I'm not opposed to support for running a purely WASM application. But the field name is awkward to me. Right now it can be read as "(content) type of inherently ambiguous files like .js and files lacking an extension". With this change, it's "(content) type of extension-less files and sometimes also of .js, you have to know from context if that applies".

I think I would even prefer if this PR would treat .js within those packages as WASM. That would be consistent with what the field currently does. It wouldn't break people who want to ship a purely WASM app and everyone else can use .mjs.

@bmeck
Copy link
Member Author

bmeck commented Jan 16, 2020 via email

@jkrems
Copy link
Contributor

jkrems commented Jan 16, 2020

Throwing on .js in that mode also seems fine. That means we can effectively still say it's the field that determines the type for all ambiguous files since few people will try to put WASM into .js in practice.

@ljharb
Copy link
Member

ljharb commented Jan 16, 2020

(See nodejs/modules#283 for a prediction of this inevitable occurrence)

@bmeck
Copy link
Member Author

bmeck commented Jan 17, 2020

That means we can effectively still say it's the field that determines the type for all ambiguous files since few people will try to put WASM into .js in practice.

I do not want the focus of the PR to be on preserving this idea. Already this is not true (see .node in "commonjs") and other features toggle in the presence of type like if to use the ESM loader for the main module. If that is a invariant we wish to make we seem to have already failed and would probably need the loader to be reverted to have that invariant in other PRs.

My desire is for the focus of this PR is solely on ensuring we have a way for WASM to be supported at the same level as other first class sources by enabling loading WASM from files without extensions.

@GeoffreyBooth
Copy link
Member

I don’t see the purpose of this. If WASM files are always parse goal Module, then they’re unaffected by the type field; and therefore we don’t need any new values for type. The .wasm extension is equivalent to .mjs or .cjs in that regard.

@jkrems
Copy link
Contributor

jkrems commented Jan 17, 2020

don’t see the purpose of this. If WASM files are always parse goal Module, then they’re unaffected by the type field; and therefore we don’t need any new values for type.

Can you clarify? WASM files can be entry points of a program and there it can be nice if we don't force the user experience to be ./app.wasm (only ever works with extension) instead of ./app (WASM files without extension supported).

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Jan 17, 2020

WASM files can be entry points of a program and there it can be nice if we don't force the user experience to be ./app.wasm (only ever works with extension) instead of ./app (WASM files without extension supported).

You’re talking about from the command line, e.g. node ./app? No, that doesn’t seem worth the confusion and added complexity around "type". I think it’s simpler to just say that Node always treats extensionless files as JavaScript, equivalent to if it were a .js file (so parse goal defined by controlling "type"). If they want node ./app or just ./app, they should create an app extensionless JavaScript file that contains import('./app.wasm').

@devsnek
Copy link
Member

devsnek commented Jan 17, 2020

that there would be a difference is a signal that something is wrong with our resolver. it should behave consistently no matter the placement of a specifier.

@jkrems
Copy link
Contributor

jkrems commented Jan 17, 2020

If they want node ./app or just ./app, they should create an app extensionless JavaScript file that contains import('./app.wasm').

You could've said the same thing for ESM: just create an extension-less CommonJS file and import() the ESM entry point. Purely WASM apps are likely to be a thing in the future. Forcing a compiler that generates otherwise self-contained WASM binaries to always write a one-line JS files to start the binary doesn't feel realistic. WASM is a first-class source type, so it has the right to exist as extension-less files imo.

@bmeck
Copy link
Member Author

bmeck commented Jan 17, 2020

I would also like to point out that WASM vs JS is seen as a security concern by some and is part of the discussion of https://github.com/tc39/proposal-module-attributes , to people of that mindset it might be undesirable to have any JS in their application.

@GeoffreyBooth
Copy link
Member

WASM is a first-class source type, so it has the right to exist as extension-less files imo.

This is really asking for an entirely new feature. Currently all extensionless files are treated as JavaScript, and type controls how they’re parsed (as CommonJS or as ESM). We don’t have a way to define as what file type extensionless files are interpreted, e.g. as JavaScript versus as native modules versus as WASM. That would probably entail a new package.json field, something like "extensionlessTreatedAs": ".js" (the current/default behavior).

I think there’s certainly no rush to add such a feature. I’ve never heard discussed that Node would treat anything other than JavaScript as equivalent first-class with JavaScript, and that’s a broader discussion that should be had before we start doing so. Having a one-line import './app.wasm' extensionless JavaScript file feels good enough for now, at least.

@devsnek
Copy link
Member

devsnek commented Jan 17, 2020

one of the main design goals of esm is to allow loading resources of any type. if we can load a format, we should be able to load it from any position that source text modules can be loaded from. if it can't be, it's an artificial limitation we should correct.

@jkrems
Copy link
Contributor

jkrems commented Jan 17, 2020

We don’t have a way to define as what file type extensionless files are interpreted, e.g. as JavaScript versus as native modules versus as WASM.

That feels like a very blurry definition of a "file type". CommonJS and ES modules are two different file types. They both happen to be text files while WASM is a binary format. But just because both are text (and their syntax has some overlap) doesn't mean they are the same file type. I can also write some CoffeeScript that parses as CommonJS and/or ES modules. Doesn't mean that CoffeeScript isn't a different file type. :)

@GeoffreyBooth
Copy link
Member

The main benefit of this is to allow files without an extension to be treated as WASM.

Why is this important? Using "type": "wasm" means that a user would also need a package.json file, so what’s the use case where the user can make a folder with at least two files (the WASM file and the pacakge.json) but can’t (or wouldn’t want to) also put an extension on the WASM file?

Having "type": "module" support extensionless files predates "exports"; it was intended to permit pretty specifiers, e.g. import shuffle from 'lodash/shuffle' where shuffle was an extensionless ES module file. Now that we have "exports", I don’t see why "type" should affect extensionless files. I especially don’t think that "type" should affect extensionless files if that means that we will need new values for "type" for every new type of entry point that Node ever supports.

We should probably just drop support for extensionless files in the ESM loader while it’s still experimental. The CommonJS loader will always load such files as CommonJS JavaScript.

@targos
Copy link
Member

targos commented Jan 18, 2020

I'm curious about how could ./app work with a WASM file. Since it's a binary format, it cannot contain a shebang, or can it?

@devsnek
Copy link
Member

devsnek commented Jan 18, 2020

@targos you can create a binfmt entry: :wasm:M::\x00\x61\x73\x6d::/path/to/node:

https://en.wikipedia.org/wiki/Binfmt_misc

@bmeck
Copy link
Member Author

bmeck commented Jan 20, 2020

#31415 is removing support for any extension-less files and supersedes this for now. A different PR can be opened if other features are found to warrant this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. modules-agenda Issues and PRs to discuss during the meetings of the Modules team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants