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

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

Merged
merged 1 commit into from Sep 22, 2016

Conversation

Projects
None yet
6 participants
@motiz88
Copy link
Contributor

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

This comment has been minimized.

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

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)

@motiz88 motiz88 referenced this pull request Sep 11, 2016

Merged

Computed class properties #4500

motiz88 added a commit to motiz88/babel-plugin-transform-hoist-nested-functions that referenced this pull request Sep 11, 2016

feat: option to hoist nested methods referencing `this` (`methods: tr鈥
鈥e`)

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 motiz88 referenced this pull request Sep 12, 2016

Merged

Hoist nested methods #5

@danez

This comment has been minimized.

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

This comment has been minimized.

Copy link

codecov-io commented Sep 12, 2016

Current coverage is 94.03% (diff: 100%)

No coverage report found for master at 015035c.

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

@motiz88

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

danharper commented Sep 12, 2016

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

@danez

This comment has been minimized.

Copy link
Member

danez commented Sep 12, 2016

Good catch. So it probably also throw with properties.

@motiz88

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

motiz88 commented Sep 12, 2016

@danez your class Bar parses as having one property.

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Sep 12, 2016

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

Can cc @jeffmo

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Sep 22, 2016

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

@jeffmo

This comment has been minimized.

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

3 checks passed

codecov/patch 100% of diff hit (target 94.03%)
Details
codecov/project Absolute coverage decreased by -2.73% but relative coverage increased by +3.22% compared to bc6aa62
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@danez

This comment has been minimized.

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

This comment has been minimized.

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 added a commit to babel/babel that referenced this pull request Sep 26, 2016

Computed class properties (#4500)
* 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 added a commit to panagosg7/babel that referenced this pull request Jan 17, 2017

Computed class properties (babel#4500)
* 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

feat: option to hoist nested methods referencing `this` (`methods: tr鈥
鈥e`)

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

feat: option to hoist nested methods referencing `this` (`methods: tr鈥
鈥e`)

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

feat: option to hoist nested methods referencing `this` (`methods: tr鈥
鈥e`)

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.