This repository has been archived by the owner. It is now read-only.

Throw error for reserved words enum and await #195

Merged
merged 3 commits into from Nov 9, 2016

Conversation

Projects
None yet
4 participants
@kaicataldo
Copy link
Member

kaicataldo commented Oct 24, 2016

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

This comment has been minimized.

Copy link

codecov-io commented Oct 24, 2016

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 kaicataldo:disallowawaitenumid branch 3 times, most recently from b6892dc to 0884f97 Oct 25, 2016

@kaicataldo

This comment has been minimized.

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

This comment has been minimized.

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 kaicataldo force-pushed the kaicataldo:disallowawaitenumid branch from 0884f97 to cff5f4b Oct 28, 2016

@kaicataldo

This comment has been minimized.

Copy link
Member Author

kaicataldo commented Oct 28, 2016

@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 👍

@@ -827,11 +827,7 @@ pp.parseObjPropValue = function (prop, startPos, startLoc, isGenerator, isAsync,

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

This comment has been minimized.

@kaicataldo

kaicataldo Oct 30, 2016

Author Member

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

@kaicataldo

This comment has been minimized.

Copy link
Member Author

kaicataldo commented Oct 30, 2016

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

This comment has been minimized.

Copy link
Member Author

kaicataldo commented Nov 3, 2016

@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 approved these changes Nov 9, 2016

@danez danez merged commit e260381 into babel:master Nov 9, 2016

3 checks passed

codecov/patch 100% of diff hit (target 96.21%)
Details
codecov/project 96.21% (+<.01%) compared to b5de37f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kaicataldo kaicataldo deleted the kaicataldo:disallowawaitenumid branch Nov 9, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.