Add variance node type and generate property variance annotations #4697

samwgoldman · 2016-10-08T00:27:05Z

Q	A
Bug fix?	no
Breaking change?	no
New feature?	yes
Deprecations?	no
Spec compliancy?	no
Tests added/pass?	yes
Fixed tickets
License	MIT
Doc PR

babel/babylon#161 adds parsing support for property variance
annotations. This PR adds the complementary support to babel-generator and tests.

babel/babylon#161 adds parsing support for property variance annotations. This PR adds the necessary node type for the new Variance node and generate support for all the positions where variance can now appear.

loganfsmyth · 2016-10-08T22:43:09Z

packages/babel-generator/src/generators/flow.js

-    this.token("+");
-  } else if (node.variance === "minus") {
-    this.token("-");
-  }


Hmm, I'm concerned that this will be a breaking change. Thought?

You're right, it is a breaking change. I can roll back that part of the change, so the TypeParameter node is unchanged, if you think that's best.

Flow 0.34, which introduces property variance, will also make the same breaking change to the TypeParameter AST.

We'll need to think through how to approach it. I just don't want to end up in a state where people get broken output or thrown exceptions.

For the generator specifically we'd probably be able to do

if (node.variance && typeof node.variance === 'object'){ this.print(node.variance, node); } else if (node.variance === "plus"){ // ...

my concern is more related to my other comment about the visitor list, because if we add variance to the visitor list for TypeParameter, people using older versions of Babylon and getting older ASTs will start getting errors because traversal will start traversing down into the string value expecting it to be an AST node.

loganfsmyth · 2016-10-09T00:21:18Z

packages/babel-generator/src/generators/classes.js

@@ -60,6 +60,7 @@ export function ClassProperty(node: Object) {
    this.print(node.key, node);
    this.token("]");
  } else {
+    this.print(node.variance, node);


Because this has added this new variance property to all of these nodes, variance needs to be added to the visitor list in https://github.com/babel/babel/blob/master/packages/babel-types/src/definitions/flow.js#L46 and all of the other node types that now have a .variance property. Otherwise, strip-flow-types will miss them and not strip them.

samwgoldman · 2016-10-10T19:01:23Z

Thank you for the thoughtful review, Logan. In retrospect, the breaking change I've made here is gratuitous. We don't need a position for the variance symbol, because it's always a single char at the very beginning of the enclosing node. I will roll back the new AST node in the Flow AST as well and just use "plus" | "minus" | null for properties as well, then update the PRs here and for babylon.

This diff also adds tests to class properties and to the flow-strip-types transform.

samwgoldman · 2016-10-11T00:39:19Z

OK, the latest update removes the breaking change and should resolve the issues raised here. Thanks again!

danez

Looks good to me. Should we make an issue to change back to variance being a Node as soon as we major bump?

samwgoldman · 2016-10-12T10:20:16Z

Yeah, an issue for that sounds good. I'm happy to do the work when the time comes :) Thanks!

loganfsmyth · 2016-10-19T05:06:02Z

@samwgoldman I realize we'll also need to add support for this to the flow-strip-types plugin at https://github.com/babel/babel/blob/master/packages/babel-plugin-transform-flow-strip-types/src/index.js#L25 to null our variance.

What would be the expected output for the comment-based representation of the type annotations?

samwgoldman · 2016-10-20T21:01:59Z

@loganfsmyth Interesting. I'm not sure I understand what the purpose of nulling out the property is there. I definitely added tests to flow-strip-types, which ensures the variance annotations are removed. Can you explain a bit more about why a change there is required?

The expected output for comment-based annotations on classes would use the /*:: */ syntax, like so:

class Base {
  /*::+*/p: ?string;
}
class Derived extends Base {
  p: string;
}

danez · 2016-10-21T12:48:06Z

I think what @loganfsmyth is referring to is the case when we mix flow with class-properties:

class C2 { 
  +p: T = e;
}

This would probably not remove the variance. flow-strip-types removes the whole property if class-properties not supported, so that's why it does not appear in the test cases. Testcase would be probably flow-strip-types + syntax-class-properties and the example above.

So it might be good in all cases to null it in flow-strip-types

codecov-io · 2016-10-21T15:55:32Z

Current coverage is 88.82% (diff: 100%)

Merging #4697 into master will increase coverage by <.01%

@@             master      #4697   diff @@
==========================================
  Files           196        196          
  Lines         13868      13874     +6   
  Methods        1430       1431     +1   
  Messages          0          0          
  Branches       3198       3198          
==========================================
+ Hits          12317      12323     +6   
  Misses         1551       1551          
  Partials          0          0

Powered by Codecov. Last update 7443f9e...c3d90e0

loganfsmyth · 2016-10-21T15:57:42Z

Sorry I didn't get time to reply yesterday. @danez is totally correct.

samwgoldman · 2016-10-21T20:03:08Z

Oooh, that makes sense—a very important use case! Thanks for catching my various missteps on this PR!

hzoo · 2016-10-24T16:35:08Z

Also it would helpful if you could writeup the changelog entry description for flow prs 😄

* master: (38 commits) chore(package): update browserify to version 13.1.1 (babel#4762) Increase test coverage (babel#4742) Make getBinding ignore labels; add Scope#getLabel, Scope#registerLabel (babel#4758) Add variance node type and generate property variance annotations (babel#4697) Add make command to delete node_modules (babel#4748) fixes [skip ci] Support ObjectExpression in static path evaluation (babel#4746) Fix replacing for-of if inside label (babel#4736) Replace `path-exists` with `fs.existsSync` (babel#4731) Avoid unnecessary +0 in transform-es2015-parameters (babel#4738) [import()] Initial support for dynamic-import (babel#4699) Fix line endings on checkout Automatically generate missing expected.js fixtures (babel#4735) Fix few typos in issue/pr templates (babel#4739) [skip ci] contributing updates [skip ci] increase git depth [skip ci] Change usage of "suite"/"test" in unit-tests to "describe"/"it" (babel#4734) Run ESLint on test files, and fix lint errors in test files (babel#4732) Add .gitattributes forcing LF line endings (babel#4730) Update tests for changed error messages in Babylon (babel#4727) ...

…bel#4697) * Add variance node type and generate property variance annotations babel/babylon#161 adds parsing support for property variance annotations. This PR adds the necessary node type for the new Variance node and generate support for all the positions where variance can now appear. * Variance is no longer a separate node type This diff also adds tests to class properties and to the flow-strip-types transform. * Add test + fix for edge case with variance and class proeprties

Add variance node type and generate property variance annotations

fcc5cec

babel/babylon#161 adds parsing support for property variance annotations. This PR adds the necessary node type for the new Variance node and generate support for all the positions where variance can now appear.

danez added PR: New Feature 🚀 A type of pull request used for our changelog categories pkg: parser labels Oct 8, 2016

loganfsmyth reviewed Oct 8, 2016

View reviewed changes

loganfsmyth reviewed Oct 9, 2016

View reviewed changes

Variance is no longer a separate node type

c3d90e0

This diff also adds tests to class properties and to the flow-strip-types transform.

danez approved these changes Oct 12, 2016

View reviewed changes

danez mentioned this pull request Oct 13, 2016

[7.0] Switch variance to be a separate node #4722

Closed

Add test + fix for edge case with variance and class proeprties

3f2828e

Merge branch 'master' into property-variance

f2c3601

hzoo merged commit 7bb430a into babel:master Oct 21, 2016

hzoo mentioned this pull request Nov 4, 2016

Babel 6 types benjamn/ast-types#162

Merged

danez mentioned this pull request Sep 3, 2017

Align flow-parser with babylon v7 facebook/flow#4789

Closed

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 7, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add variance node type and generate property variance annotations #4697

Add variance node type and generate property variance annotations #4697

samwgoldman commented Oct 8, 2016 •

edited

Loading

loganfsmyth Oct 8, 2016

samwgoldman Oct 9, 2016

loganfsmyth Oct 9, 2016

loganfsmyth Oct 9, 2016

loganfsmyth Oct 9, 2016

samwgoldman commented Oct 10, 2016

samwgoldman commented Oct 11, 2016

danez left a comment

samwgoldman commented Oct 12, 2016

loganfsmyth commented Oct 19, 2016 •

edited

Loading

samwgoldman commented Oct 20, 2016

danez commented Oct 21, 2016 •

edited

Loading

codecov-io commented Oct 21, 2016 •

edited

Loading

loganfsmyth commented Oct 21, 2016

samwgoldman commented Oct 21, 2016

hzoo commented Oct 24, 2016

Add variance node type and generate property variance annotations #4697

Add variance node type and generate property variance annotations #4697

Conversation

samwgoldman commented Oct 8, 2016 • edited Loading

loganfsmyth Oct 8, 2016

Choose a reason for hiding this comment

samwgoldman Oct 9, 2016

Choose a reason for hiding this comment

loganfsmyth Oct 9, 2016

Choose a reason for hiding this comment

loganfsmyth Oct 9, 2016

Choose a reason for hiding this comment

loganfsmyth Oct 9, 2016

Choose a reason for hiding this comment

samwgoldman commented Oct 10, 2016

samwgoldman commented Oct 11, 2016

danez left a comment

Choose a reason for hiding this comment

samwgoldman commented Oct 12, 2016

loganfsmyth commented Oct 19, 2016 • edited Loading

samwgoldman commented Oct 20, 2016

danez commented Oct 21, 2016 • edited Loading

codecov-io commented Oct 21, 2016 • edited Loading

Current coverage is 88.82% (diff: 100%)

loganfsmyth commented Oct 21, 2016

samwgoldman commented Oct 21, 2016

hzoo commented Oct 24, 2016

samwgoldman commented Oct 8, 2016 •

edited

Loading

loganfsmyth commented Oct 19, 2016 •

edited

Loading

danez commented Oct 21, 2016 •

edited

Loading

codecov-io commented Oct 21, 2016 •

edited

Loading