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

Breaking: acorn to 8.1.0 (fixes #470) #473

Merged
merged 4 commits into from Apr 16, 2021
Merged

Breaking: acorn to 8.1.0 (fixes #470) #473

merged 4 commits into from Apr 16, 2021

Conversation

aladdin-add
Copy link
Member

@aladdin-add aladdin-add commented Apr 9, 2021

@nzakas nzakas changed the title Upgrade: acorn to 8.1.0 Upgrade: acorn to 8.1.0 (fixes #470, fixes #472) Apr 10, 2021
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Can we add tests that show the issues are resolved?

@mdjermanovic
Copy link
Member

Upgrading to acorn 8 is a breaking change for espree, as discussed in #456.

#469 also upgrades to acorn 8, so maybe we could just make an additional chore PR with tests for #470 and #472.

@aladdin-add aladdin-add changed the title Upgrade: acorn to 8.1.0 (fixes #470, fixes #472) breaking: acorn to 8.1.0 (fixes #470, fixes #472) Apr 12, 2021
@aladdin-add aladdin-add changed the title breaking: acorn to 8.1.0 (fixes #470, fixes #472) Breaking: acorn to 8.1.0 (fixes #470, fixes #472) Apr 12, 2021
@aladdin-add
Copy link
Member Author

I think this is ready to be reviewed and merged. 🚀

But if #469 get merged before this one. I will update the title to "Chore" as @mdjermanovic suggested.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM

tests/lib/parse.js Outdated Show resolved Hide resolved
tests/lib/parse.js Outdated Show resolved Hide resolved
tests/lib/parse.js Outdated Show resolved Hide resolved
Comment on lines 117 to 119
assert.throws(() => {
espree.parse("async () => await 5 ** 6;");
});
Copy link
Member

Choose a reason for hiding this comment

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

Similar to the above, we should set a higher ecmaVersion to confirm that #472 has been fixed, because this code with the default ecmaVersion already has syntax errors aside from await 5 ** 6.

Actually, it seems that this particular example doesn't throw in the latest Acorn 8.

Copy link
Member Author

Choose a reason for hiding this comment

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

interesting! if adding a {}, it will throw an error.

> acorn.parse('async() => { await 5 ** 6 }', {ecmaVersion: 10})
Uncaught [SyntaxError: Unexpected token (1:21)
] {
  pos: 21,
  loc: Position { line: 1, column: 21 },
  raisedAt: 23
}

sounds like a bug in acorn?

Copy link
Member Author

Choose a reason for hiding this comment

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

just removed the case, as it was not fixed in latest acorn. :)

aladdin-add and others added 2 commits April 15, 2021 14:33
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@aladdin-add aladdin-add changed the title Breaking: acorn to 8.1.0 (fixes #470, fixes #472) Breaking: acorn to 8.1.0 (fixes #470) Apr 15, 2021
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@nzakas nzakas merged commit 995d2a8 into master Apr 16, 2021
@nzakas nzakas deleted the upgrade/acorn branch April 16, 2021 17:27
@robpalme robpalme mentioned this pull request Apr 26, 2021
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.

Should throw on invaid (a = 1) = t
3 participants