Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flow] Explicit inexact objects with ... #8884

Merged
merged 7 commits into from Oct 29, 2018

Conversation

jbrown215
Copy link
Contributor

@jbrown215 jbrown215 commented Oct 16, 2018

Q                       A
Fixed Issues? No
Patch: Bug Fix?
Major: Breaking Change? No
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes? No
License MIT

In facebook/flow@1ac9130, flow started parsing {foo: number, ...} as an inexact object. This is part of a larger effort to move to parsing object types as exact by default.

... can only appear at the end of the props list, and it cannot appear inside {||}. Additionally, it can only appear inside object types, and not interfaces, declare classes, or declare interfaces. ... also supports trailing commas.

For any object type, but not in declare class, declare interface, or interface, the ObjectTypeAnnotation representing it now has an inexact property. This property is true if and only if ... was used to indicate the inexactness. To minimize confusion, it is best to read exact as explicitly exact (containing curlybars) and inexact as explicitly inexact (containing ...). This mimics the behavior in Flow's estree output facebook/flow@231256c. In a few releases, we will get rid of the exact flag entirely, which will be a breaking change to other tools.

Tests had to be re-recorded to include the inexact flag. I separated the re-recording of old tests into a separate commit for reviewing ease.

Let me know if there is anything else I need to update in this change.

@babel-bot
Copy link
Collaborator

babel-bot commented Oct 16, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/9276/

@existentialism existentialism added area: flow PR: New Feature 🚀 A type of pull request used for our changelog categories labels Oct 18, 2018
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Apart from some nits, looks good 👍

packages/babel-parser/src/plugins/flow.js Show resolved Hide resolved
packages/babel-parser/src/plugins/flow.js Show resolved Hide resolved
case tt.braceBarR:
this.flowObjectTypeSemicolon();
switch (this.state.type) {
case tt.braceR:
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think that this code would be more readable without nested switch statemets:

let isInexactToken = this.eat(tt.comma) || this.eat(tt.semi);

if (this.match(tt.braceR)) {
  if (allowInexact) return true;
  this.unexpected(null, "Explicit inexact syntax is only allowed inside inexact objects");
}

if (this.match(tt.braceBarR)) {
  this.unexpected(null, "Explicit inexact syntax cannot appear inside an explicit exact object type");
}

if (isInexactToken) {
  this.unexpected(null, "Explicit inexact syntax must appear at the end of an inexact object");
}

node.argument = this.flowParseType();
return this.finishNode(node, "ObjectTypeSpreadProperty");

Do what you prefer though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this better, will follow up

nicolo-ribaudo
nicolo-ribaudo previously approved these changes Oct 20, 2018
case tt.comma:
case tt.semi:
case tt.braceR:
case tt.braceBarR: {
Copy link
Member

Choose a reason for hiding this comment

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

I think that you can also remove this outer switch, since the ifs already handle only these four cases.

null,
"Explicit inexact syntax cannot appear inside an explicit exact object type",
);
return false;
Copy link
Member

Choose a reason for hiding this comment

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

Those retuens aren't needed, since this.unexpected already throws. If you want to be explicit, I think that it would be more correct to use throw this.unexpected.

@nicolo-ribaudo nicolo-ribaudo dismissed their stale review October 20, 2018 19:44

Wrong button in the last review 😅

@jbrown215
Copy link
Contributor Author

@nicolo-ribaudo, thanks for the feedback!

I'll backport this to 6.xx as well, since I assume there's still a bunch of flow projects out there that haven't upgraded to babel 7 yet.

@jbrown215
Copy link
Contributor Author

Actually, it's not clear to me where to start with backporting this change. The babylon repo is read-only and the 6.x readme/contributing.md doesn't say where to put up new PRs. The 6.x branch here doesn't have a babylon or babel-parser package. Is there any follow up work I need to do here?

@nicolo-ribaudo
Copy link
Member

We are not going to release new Babel 6 version, especially for new features.

@jbrown215
Copy link
Contributor Author

Any idea when the next babel release is / if this will be in it?

@existentialism existentialism merged commit e4929e1 into babel:master Oct 29, 2018
@existentialism
Copy link
Member

@jbrown215 no exact date... soon though? :)

@jbrown215 jbrown215 deleted the ellipsis_inexact branch November 1, 2018 17:20
mAAdhaTTah added a commit to valtech-nyc/babel that referenced this pull request Nov 3, 2018
* master: (222 commits)
  Set correct methods name
  Use toPropertyKey in the "decorate" helper
  Allow function types in type params within arrow return types (babel#8954)
  Fix message when plugin of a wrong type is passed (babel#8950)
  rename colliding let bindings with for loop init (babel#8937)
  edge incomplete support for arrow destructuring (babel babel#8349) (babel#8926)
  fix single-arg async arrows when retainLines=true (babel#8868)
  [flow] Explicit inexact objects with `...` (babel#8884)
  Update preset-env data (babel#8898)
  Treat break inside block inside loop (babel#8914)
  fixed "source map" formatting in comment (babel#8878) [skip ci]
  fix typo in contributing guidelines (babel#8901) [skip ci]
  fix: Expression x === 'y' && '' should not evaluate to undefined. (babel#8880)
  fixed an extra word
  Fixes babel#8865 (babel#8866)
  v7.1.4
  v7.1.3
  Bump Babel deps (babel#8770)
  flow-bin@0.82.0 (babel#8832)
  Insertafter jsx fix (babel#8833)
  ...

# Conflicts:
#	packages/babel-parser/src/tokenizer/index.js
#	packages/babel-parser/test/fixtures/experimental/class-private-properties/failure-numeric-literal/options.json
#	packages/babel-parser/test/fixtures/experimental/pipeline-operator/invalid-proposal/options.json
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: flow outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants