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

feat: add support for TLA on node 14.3 and newer #7

Merged
merged 2 commits into from Oct 31, 2020
Merged

Conversation

SimenB
Copy link
Collaborator

@SimenB SimenB commented Oct 31, 2020

See babel/babel#12292

https://nodejs.org/en/blog/release/v14.3.0/

I'm unable to make this pass the tests, tho :( Weirdly, copying in the code here makes TLA work in a test in Jest repo, but I keep getting parser errors here. I tried updating @babel/core to latest just in case, but that didn't make a difference.

One interesting observation is that in Jest it only works if I use loadPartialConfig (which is what we use by default) - if I use loadOptions instead it fails with the same error it fails with here. However, I'm not having any luck replicating that behavior in this preset

@@ -45,8 +45,13 @@ for (const [name, cases] of Object.entries(tests)) {
// runtime. It is supported starting from 10.4, so we can check the version.
const major = parseInt(process.versions.node, 10);
const minor = parseInt(process.versions.node.match(/^\d+\.(\d+)/)[1], 10);
if (major > 10 || (major === 10 && minor > 4)) {
if (major > 10 || (major === 10 && minor >= 4)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug fix looking at the comment above

@@ -15,7 +15,7 @@ jobs:

strategy:
matrix:
node-version: [10.19, 12.4, 12.8, 13.11, 14]
node-version: [10.19, 12.4, 12.8, 13.11, 14.3, 14, 15]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess 15 doesn't matter, but anyways

@nicolo-ribaudo
Copy link
Owner

For the tests, maybe passing sourceType: "module" to

babel.parseSync(code, { configFile: false, presets: [thisPreset] });
is enough.

@SimenB
Copy link
Collaborator Author

SimenB commented Oct 31, 2020

sourceType: "module" makes no difference

@SimenB
Copy link
Collaborator Author

SimenB commented Oct 31, 2020

There's something else different between the Jest repo and this repo. If I don't have the syntax plugin in the preset in the Jest repo, I get this error

SyntaxError: /Users/simen/repos/jest/e2e/native-esm/__tests__/native-esm-tla.test.js: Can not use keyword 'await' outside an async function (8:14)

But the error I get in this repo - with or without the syntax plugin - is

SyntaxError: unknown: await is a reserved word (1:0)

I would have thought just using a newer version of @babel/core would fix that as it seems that error was improved, but this is using "@babel/core": "^7.0.0" in devDeps and refreshing the lockfile so everything should be at the latest version. This is with the follow parse call - I added a bunch of stuff just in case

babel.parseSync(code, {
  configFile: false,
  presets: [thisPreset],
  sourceType: "module",
  caller: {
    name: "syntax-preset-tester",
    supportsStaticESM: true,
    supportsDynamicImport: true,
    supportsTopLevelAwait: true,
    supportsExportNamespaceFrom: true,
  },
})

@nicolo-ribaudo
Copy link
Owner

Oh I checked the lockfile and it's still using @babel/parser 7.0.0, which doesn't support TLA

@SimenB
Copy link
Collaborator Author

SimenB commented Oct 31, 2020

Ah, I missed the resolutions entry! Unlocking @babel/core dev dep and @babel/parser resolution and then regenerating the lockfile makes the test pass. Thoughts on how to deal with it?

EDIT: Just removing the resolution work, the dev dep doesn't matter

@nicolo-ribaudo
Copy link
Owner

Ok so, the problem is that for the other tests we still need @babel/parser@7.0.0, otherwise some syntax features are enabled by default and we wouldn't be actually testing this preset.

@arcanis If I have this package.json

  "devDependencies": {
    "@babel/core@7.0.0": "npm:@babel/core@7.0.0",
    "@babel/core@7.9.0": "npm:@babel/core@7.9.0"
  },

Can I use resolutions to force @babel/core@7.0.0 to depend on @babel/parser@7.0.0, and @babel/core@7.9.0 on @babel/parser@7.9.0? I tried adding "@babel/core@7.0.0/@babel/parser": "7.0.0" but it doesn't seem to work.

@nicolo-ribaudo
Copy link
Owner

I think I found a fix.

@nicolo-ribaudo
Copy link
Owner

Tests are passing ✌️

@SimenB
Copy link
Collaborator Author

SimenB commented Oct 31, 2020

Yay! 🎉

@nicolo-ribaudo
Copy link
Owner

I'll release 1.0.0, since this project is "stable enough"

@SimenB
Copy link
Collaborator Author

SimenB commented Oct 31, 2020

wonderful, thanks!

@SimenB
Copy link
Collaborator Author

SimenB commented Oct 31, 2020

Bumped in jestjs/jest#10747, will release in v26 either this weekend or monday

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

Successfully merging this pull request may close these issues.

None yet

2 participants