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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

update acorn@^7.3.1 for optional chaining support #5013

Merged
merged 6 commits into from Jun 27, 2020

Conversation

tanhauhau
Copy link
Member

@tanhauhau tanhauhau commented Jun 14, 2020

TODO:

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

  • Run the tests tests with npm test or yarn test)

@tanhauhau tanhauhau marked this pull request as draft June 14, 2020 04:33
@tanhauhau tanhauhau changed the title update acorn@^7.3.1 update acorn@^7.3.1 for optional chaining support Jun 14, 2020
@@ -201,7 +201,7 @@ export default class Expression {
scope = map.get(node);
}

if (is_reference(node, parent)) {
if (node.type === 'Identifier' && is_reference(node, parent)) {
Copy link
Member Author

@tanhauhau tanhauhau Jun 14, 2020

Choose a reason for hiding this comment

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

not sure why in is-reference https://github.com/Rich-Harris/is-reference/blob/ca593f330bff5660efec747ddc5973e875e2f923/module.js#L2-L4, MemberExpression is treated as reference for certain scenario, which may lead to lost of optional chaining, because of some code generation logic in Svelte

return nodes.reduce((lhs, rhs) => x`${lhs}.${rhs}`);

@Conduitry
Copy link
Member

I've just published code-red@0.1.3.

@tanhauhau tanhauhau marked this pull request as ready for review June 24, 2020 13:52
@Conduitry
Copy link
Member

It looks like there's still a TS build error here.

I'm not sure what I think about bind:value={a?.b}. What would that do if a were nullish? Nothing? Set it to { b }? Maybe for now it should be a compilation error?

@tanhauhau
Copy link
Member Author

the TS error was due to 2 @types/estree instance, as is-reference and code-red installed 2 different absolute version of @types/estree.

created a PR for that Rich-Harris/is-reference#9

Maybe for now it should be a compilation error?

okay, it is a compilation error for now, since currently only MemberExpression and Identifier is supported, and optional chain is a ChainExpression .

@qm3ster
Copy link

qm3ster commented Jun 26, 2020

Does this branch in the current state enable compiling ?. and ??? Because I tried to git depend on it and it didn't for me.
Not rushing anyone, just assumed it would from the checkmarks in the description and the fact this is marked as closing #1972.
This includes adding lang="typescript". Does it need a specific typescript config?

@Conduitry
Copy link
Member

@tanhauhau It took a little npm wrangling to get it to update just the relevant bits, but here's the diff I have right now for the lockfile:

diff --git a/package-lock.json b/package-lock.json
index 1bb4bbfff..25b0b6703 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -167,9 +167,9 @@
       "dev": true
     },
     "@types/estree": {
-      "version": "0.0.39",
-      "resolved": "https://registry.npmjs.org/@types/estree/-/estree-0.0.39.tgz",
-      "integrity": "sha512-EYNwp3bU+98cpU4lAWYYL7Zz+2gryWH1qbdDTidVd6hkiR6weksdbMadyXKXNPEkQFhXM+hVO9ZygomHXp+AIw==",
+      "version": "0.0.45",
+      "resolved": "https://registry.npmjs.org/@types/estree/-/estree-0.0.45.tgz",
+      "integrity": "sha512-jnqIUKDUqJbDIUxm0Uj7bnlMnRm1T/eZ9N+AVMqhPgzrba2GhGG5o/jCTwmdPK709nEZsGoMzXEDUjcXHa3W0g==",
       "dev": true
     },
     "@types/is-windows": {
@@ -2250,12 +2250,12 @@
       "dev": true
     },
     "is-reference": {
-      "version": "1.1.4",
-      "resolved": "https://registry.npmjs.org/is-reference/-/is-reference-1.1.4.tgz",
-      "integrity": "sha512-uJA/CDPO3Tao3GTrxYn6AwkM4nUPJiGGYu5+cB8qbC7WGFlrKZbiRo7SFKxUAEpFUfiHofWCXBUNhvYJMh+6zw==",
+      "version": "1.2.1",
+      "resolved": "https://registry.npmjs.org/is-reference/-/is-reference-1.2.1.tgz",
+      "integrity": "sha512-U82MsXXiFIrjCK4otLT+o2NA2Cd2g5MLoOVXUZjIOhLurrRxpEXzI8O0KZHr3IjLvlAH1kTPYSuqer5T9ZVBKQ==",
       "dev": true,
       "requires": {
-        "@types/estree": "0.0.39"
+        "@types/estree": "*"
       }
     },
     "is-regex": {

I'm still seeing type errors when building though - three messages about src/compiler/compile/nodes/shared/Context.ts.

@Conduitry
Copy link
Member

With the above lockfile changes, and forcing Context.ts to compile despite the remaining type errors, this does seem to be working. However, the REPL itself won't work yet, because the version of Rollup used there (1.27.0) does not support these syntax features. So the compiler produces the correct output but then the bundler chokes on it. I've opened sveltejs/svelte-repl#122 which would be nice to get to in the immediate future but I don't think it should block this PR.

@tanhauhau
Copy link
Member Author

tanhauhau commented Jun 27, 2020

I've applied your package-lock.json diff, and fixed the type errors!

@Conduitry Conduitry merged commit 2450dd1 into sveltejs:master Jun 27, 2020
@tanhauhau tanhauhau deleted the tanhauhau/update-acorn branch June 27, 2020 14:52
taylorzane pushed a commit to taylorzane/svelte that referenced this pull request Dec 17, 2020
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.

Compiler error when referencing import.meta.url in svelte scripts '?' navigation operator
3 participants