Skip to content
This repository was archived by the owner on May 19, 2018. It is now read-only.

Throw error for reserved words enum and await #195

Merged
danez merged 3 commits into from
Nov 9, 2016
Merged

Throw error for reserved words enum and await #195

danez merged 3 commits into from
Nov 9, 2016

Conversation

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Oct 24, 2016 •
edited
Loading

Refs #134

Removed check and tests for renamed destructuring assignments. See comments below.
This PR ended up being a bit bigger because Babylon currently doesn't check renamed destructured values to see if the new name is a reserved word/keyword. So before this PR, the following would not warn:

function foo({ bar: enum }) {}
function foo({ bar: protected }) {} // in strict mode
const { foo: enum } = bar();
const { foo: protected } = bar(); // in strict mode

To ensure the reserved words error is thrown everywhere, I also implemented this check. Is this too many changes in one PR?

@codecov-io
Copy link

codecov-io commented Oct 24, 2016 •
edited
Loading

Current coverage is 96.21% (diff: 100%)

No coverage report found for master at 572bc9c.

Powered by Codecov. Last update 572bc9c...4aba3d1

@kaicataldo kaicataldo force-pushed the disallowawaitenumid branch 3 times, most recently from b6892dc to 0884f97 Compare October 25, 2016 02:46
@kaicataldo
Copy link
Member Author

kaicataldo commented Oct 25, 2016

Looks like I might also need to write some more tests to get higher coverage

@hzoo
Copy link
Member

hzoo commented Oct 25, 2016

I think we should make the check renamed destructured values part as a separate PR since there's a lot of changes to review and they can be independent. Easir to merge both then?

And maybe consolidate the error messages so it's the basically the same thing - not being able to use reserved words and in strict mode or not

@kaicataldo
Copy link
Member Author

kaicataldo commented Oct 28, 2016 •
edited
Loading

@hzoo How do these changes look? I removed the check and tests for renamed destructuring assignments and centralized the reserved word checking logic. Wanted to double check before fixing all the tests (129 failing tests now because the error message has changed to the more generic one) :)

I definitely think this will be easier to maintain 👍

kaicataldo
kaicataldo commented Oct 30, 2016
View reviewed changes
src/parser/expression.js Outdated

if (!prop.computed && prop.key.type === "Identifier") {
if (isPattern) {
if (this.isKeyword(prop.key.name)) {
Copy link
Member Author

@kaicataldo kaicataldo Oct 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Moved this into the checkReservedWord() method to put all the reserved word checks in there

@kaicataldo
Copy link
Member Author

kaicataldo commented Oct 30, 2016 •
edited
Loading

Updated! Let me know if there's anything else that needs to be done. Will follow this PR up with another one that uses the checkReservedWords() method to check for reserved words for renamed destructed assignments (since those are not currently checked).

@kaicataldo kaicataldo changed the title Throw error for reserved words enum and await when source type is module Throw error for reserved words enum and await Oct 30, 2016
@kaicataldo
Copy link
Member Author

kaicataldo commented Nov 3, 2016 •
edited
Loading

@danez @motiz88 Thoughts on this approach? Let me know if there's anything else I can do to land this.

As a follow up I'd like to check renamed destructuring assignments for reserved words (currently not checked) as well as look into the other half of this issue (allowing let as an identifier name in non-strict mode). Should be easy to do with the foundation set up in this PR.

@danez danez merged commit e260381 into babel:master Nov 9, 2016
@kaicataldo kaicataldo deleted the disallowawaitenumid branch November 9, 2016 15:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@danez danez danez approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

spec-violation Tag: Breaking Change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kaicataldo @codecov-io @hzoo @danez