-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
update acorn@^7.3.1 for optional chaining support #5013
Conversation
@@ -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)) { |
There was a problem hiding this comment.
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}`); |
I've just published |
f07d8ab
to
59acd4b
Compare
7d91315
to
59acd4b
Compare
It looks like there's still a TS build error here. I'm not sure what I think about |
the TS error was due to 2 created a PR for that Rich-Harris/is-reference#9
okay, it is a compilation error for now, since currently only |
Does this branch in the current state enable compiling |
@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. |
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. |
94ec751
to
c98c5e4
Compare
I've applied your package-lock.json diff, and fixed the type errors! |
TODO:
code-red
support optional chaining Rich-Harris/code-red#45 (merged, but pending new version fromcode-red
)update(apparently, this is not a dependency of svelte 🤔 )@types/estree
forestree
type definition [estree] optional chain, nullish coallesce DefinitelyTyped/DefinitelyTyped#45470bind:value={a?.b}
?Before submitting the PR, please make sure you do the following
npm run lint
!)Tests
npm test
oryarn test
)