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

Add support for computed class property names (#120) #121

Merged
merged 1 commit into from Sep 22, 2016

Conversation

motiz88
Copy link
Contributor

@motiz88 motiz88 commented Sep 11, 2016

First time contributor here - someone please check this for sanity 😄

I think this implements #120 quite completely. I'm working on a PR for the corresponding changes required in Babel, tracked by babel/babel#4499.

@motiz88
Copy link
Contributor Author

motiz88 commented Sep 11, 2016

The CI failures (on Node 5 only, out of the whole build matrix) appear to be unrelated to the PR's content. (Right?)

motiz88 added a commit to motiz88/babel that referenced this pull request Sep 11, 2016
** Depends on babel/babylon#121 **

* `babel-types`: Add `computed` field to `ClassProperty`

* `babel-plugin-transform-class-properties`: handle computed property names correctly

* `babel-generator`: add tests for class properties (computed/literal, static/instance)
motiz88 added a commit to motiz88/babel-plugin-transform-hoist-nested-functions that referenced this pull request Sep 11, 2016
…ue`)

See README.md for documentation.

This feature is blocked on the following Babel PRs/issues:
* babel/babel#4500
* babel/babylon#121
* babel/babel#4337
* babel/babel#4230 (partially)
@danez
Copy link
Member

danez commented Sep 12, 2016

Thanks for contributing.

This looks really good, the only thing that I'm worried about is how the ambiguous cases are handled. For example this class:

class Bar {
   [x] = blub
   ['y'];
}

Does it contain two or one properties? I would assume one with a value of blub['y']. But I'm not sure what the spec says about this if it does.
What would your changes produce for this example?

@codecov-io
Copy link

Current coverage is 94.03% (diff: 100%)

No coverage report found for master at 015035c.

Powered by Codecov. Last update 015035c...f09ab3f

@motiz88
Copy link
Contributor Author

motiz88 commented Sep 12, 2016

My immediate answer is that ASI applies like everywhere else (and there are existing tests for literal-named properties reflecting this). Need to do a little bit of research before I can add more info.

@danez
Copy link
Member

danez commented Sep 12, 2016

I found this
tc39/proposal-class-public-fields#25

Not sure what the current status is, but semicolons might be required in the case i posted.

@danharper
Copy link
Member

There's some existing tests, which cover the ASI rules in this case for computed methods.

@danez
Copy link
Member

danez commented Sep 12, 2016

Good catch. So it probably also throw with properties.

@motiz88
Copy link
Contributor Author

motiz88 commented Sep 12, 2016

There's also tc39/proposal-class-public-fields/pull/45 (an open PR at the moment) which suggests the semicolon is not in the spec, but IMHO it's safe to assume it will be (given past discussions and the nontrivial ambiguity otherwise).

@motiz88
Copy link
Contributor Author

motiz88 commented Sep 12, 2016

@danez your class Bar parses as having one property.

@hzoo
Copy link
Member

hzoo commented Sep 12, 2016

Yeah ref babel/babel#3225 and revert pr babel/babel#3332

Can cc @jeffmo

@hzoo
Copy link
Member

hzoo commented Sep 22, 2016

Looks fine to me? We made semicolons necessary before but it was reverted

@jeffmo
Copy link
Contributor

jeffmo commented Sep 22, 2016

The tests look right to me

@hzoo hzoo merged commit 4e1fbd4 into babel:master Sep 22, 2016
@danez
Copy link
Member

danez commented Sep 22, 2016

@jeffmo what will the spec say about this case? Or is it not yet decided?

class Bar {
   [x] = foo
   ['y'];
}

@jeffmo
Copy link
Contributor

jeffmo commented Sep 22, 2016

@danez: Grammatically, that would be the same as:

class Bar {
  [x] = foo['y'];
}

This is all just the practical fallout of supporting ASI in the language.

danez pushed a commit to babel/babel that referenced this pull request Sep 26, 2016
* Support computed class property names (#4499)

** Depends on babel/babylon#121 **

* `babel-types`: Add `computed` field to `ClassProperty`

* `babel-plugin-transform-class-properties`: handle computed property names correctly

* `babel-generator`: add tests for class properties (computed/literal, static/instance)

* doc: Update babel-types with ClassProperty.computed

* chore(package): update babylon to v6.11.0

* babel-types: move ClassProperty.computed to be last builder arg
panagosg7 pushed a commit to panagosg7/babel that referenced this pull request Jan 17, 2017
* Support computed class property names (babel#4499)

** Depends on babel/babylon#121 **

* `babel-types`: Add `computed` field to `ClassProperty`

* `babel-plugin-transform-class-properties`: handle computed property names correctly

* `babel-generator`: add tests for class properties (computed/literal, static/instance)

* doc: Update babel-types with ClassProperty.computed

* chore(package): update babylon to v6.11.0

* babel-types: move ClassProperty.computed to be last builder arg
motiz88 added a commit to motiz88/babel-plugin-transform-hoist-nested-functions that referenced this pull request Apr 2, 2017
…ue`)

See README.md for documentation.

This feature is blocked on the following Babel PRs/issues:
* babel/babel#4500
* babel/babylon#121
* babel/babel#4337
* babel/babel#4230 (partially)
motiz88 added a commit to motiz88/babel-plugin-transform-hoist-nested-functions that referenced this pull request Apr 2, 2017
…ue`)

See README.md for documentation.

This feature is blocked on the following Babel PRs/issues:
* babel/babel#4500
* babel/babylon#121
* babel/babel#4337
* babel/babel#4230 (partially)
motiz88 added a commit to motiz88/babel-plugin-transform-hoist-nested-functions that referenced this pull request Apr 2, 2017
…ue`)

See README.md for documentation.

This feature is blocked on the following Babel PRs/issues:
* babel/babel#4500
* babel/babylon#121
* babel/babel#4337
* babel/babel#4230 (partially)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants