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

Import assertions #5391

Merged
merged 9 commits into from
Jan 20, 2022
Merged

Conversation

GeoffreyBooth
Copy link
Collaborator

This PR adds support for the import assertions syntax, which permits import or import() of JSON files (browsers and Node) and CSS files (browsers). It looks like this (identical in CoffeeScript and JavaScript):

import dates from './calendar.json' assert { type: 'json' }
export { version } from './package.json' assert { type: 'json' }

The Node feature is flagged behind --experimental-json-modules; I was partly responsible for adding it. The syntax is supported without flags in at least Chrome. You can test in latest Node by running the tests after setting export NODE_OPTIONS=--experimental-json-modules, or run cake build:browser to update the browser build and run http-server docs/v2 and go to http://127.0.0.1:8080/test.html in Chrome. Only the import() syntax is tested by running, since we don’t yet have support for ESM syntax in our tests (import() is supported in CommonJS and classic Script modes for Node and browsers, respectively) and so the static import and export syntaxes are tested via string comparisons, like our existing import and export tests. (I’d love to get proper ESM support in our tests, but that’s another PR.)

I followed the AST I saw in https://astexplorer.net/ for @babel/parser. @helixbass please let me know if it looks okay.

@GeoffreyBooth
Copy link
Collaborator Author

Does anyone mind reviewing this? @vendethiel? @edemaine?

@vendethiel
Copy link
Collaborator

Maybe a regression test for an identifier being assert if we don’t have that and a test for an empty assert block

@GeoffreyBooth
Copy link
Collaborator Author

Maybe a regression test for an identifier being assert if we don’t have that and a test for an empty assert block

Like import assert from 'foo'? and import foo from 'bar' assert {}?

@vendethiel
Copy link
Collaborator

Yes, and assert = 1

@GeoffreyBooth
Copy link
Collaborator Author

Yes, and assert = 1

Done.

@GeoffreyBooth GeoffreyBooth merged commit f557c05 into jashkenas:main Jan 20, 2022
@GeoffreyBooth GeoffreyBooth deleted the import-assertions branch January 20, 2022 19:40
@edemaine
Copy link
Contributor

It looks like the new tests somehow broke cake test. I get

TypeError [ERR_INVALID_MODULE_SPECIFIER]: Invalid module "data:application/json,{"ofLife":42}" has an unsupported MIME type "application/json"

It looks like I need to add --experimental-json-modules but I don't see the CI doing that. Am I missing something? I'm running Node 17.3.0 on Windows 10.

@GeoffreyBooth
Copy link
Collaborator Author

cake test passes for me. Not sure why you’d see differently? The other one that’s good to run is cake test:browser, which fires up headless Chrome to run the browser tests. In this case Chrome supports the import assertions syntax without a flag, so those tests get run in that environment (which runs in CI). The import assertion tests are currently getting skipped in Node, per:

skipUnless 'async () => { await import(\'data:application/json,{"foo":"bar"}\', { assert: { type: "json" } }) }', ['import_assertions.coffee']

Once JSON modules are unflagged in Node, which should happen soon, those tests will automatically start running in Node too, and they’ll pass. I tested by setting the flag ahead of time via export NODE_OPTIONS=--experimental-json-modules.

@edemaine
Copy link
Contributor

My guess (based on the experiments below) is that the skipUnless test is checking whether NodeJS can parse the assert syntax, but isn't testing whether NodeJS will import a JSON module, and Node 17.3.0 supports the former but not the latter without setting NODE_OPTIONS. Perhaps you're using a different Node version than I am, one that doesn't support assert syntax, so the test gets skipped?

> process.version
'v17.3.0'
> new Function('async () => { await import(\'data:application/json,{"foo":"bar"}\', { assert: { type: "json" } }) }')
[Function: anonymous]
> new Function('import(\'data:application/json,{"foo":"bar"}\', { assert: { type: "json" } })')
[Function: anonymous]
> await new Function('return import(\'data:application/json,{"foo":"bar"}\', { assert: { type: "json" } })')()
Uncaught:
TypeError [ERR_INVALID_MODULE_SPECIFIER]: Invalid module "data:application/json,{"foo":"bar"}" has an unsupported MIME type "application/json"
    at __node_internal_captureLargerStackTrace (node:internal/errors:464:5)
    at new NodeError (node:internal/errors:371:5)
    at ESMLoader.load (node:internal/modules/esm/loader:380:13)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async ESMLoader.moduleProvider (node:internal/modules/esm/loader:280:47)
    at async link (node:internal/modules/esm/module_job:70:21) {
  code: 'ERR_INVALID_MODULE_SPECIFIER'
}

Here's the output I get with NODE_OPTIONS=--experimental-json-modules:

> await new Function('return import(\'data:application/json,{"foo":"bar"}\', { assert: { type: "json" } })')()
[Module: null prototype] { default: { foo: 'bar' } }
> (node:41788) ExperimentalWarning: Importing JSON modules is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)

So now we just need to figure out a different skipUnless test... But I don't see how to do it when wrapping in new Function, or more generally how to do it in a synchronous setting. 😕

@GeoffreyBooth
Copy link
Collaborator Author

Our CI doesn’t include 17 as it’s unstable; we stop at 16. I was testing in 16. JSON modules will get unflagged before 18, so I don’t think it’ll matter much whether this works in 17.

For now either just set NODE_OPTIONS=--experimental-json-modules beforehand or develop in Node 16.

@GeoffreyBooth GeoffreyBooth mentioned this pull request Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants