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

Should not remove class fields when using flow #10039

Closed
nicolo-ribaudo opened this issue May 29, 2019 · 9 comments · Fixed by #12457
Closed

Should not remove class fields when using flow #10039

nicolo-ribaudo opened this issue May 29, 2019 · 9 comments · Fixed by #12457
Assignees
Labels
claimed good first issue Has PR i: bug outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Milestone

Comments

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented May 29, 2019

If the flow plugin runs before the class properties one (or if the class properties plugin isn't used), we remove uninitialize class fields:

class Foo { x: string }

currently becomes

class Foo {}

while it should be

class Foo { x }

If someone still wants the old behavior, they can either use flow comments (as offically recommended), or use the ignoreUninitialized option which will soon be added to the class properties plugin (#9141).


Is there anyone who wants to contribute for the first time to Babel? 🙂

NOTE ⚠️⚠️⚠️ Since this is a breaking change, we won't merge the PR for a long time (before Babel 8), and it might seem that we are ignoring it.

If you don't know how to clone Babel, follow these steps: (you need to have make and yarn available on your machine).

  1. Write a comment there to know other possible contributors that you are working on this bug.
  2. Fork the repo
  3. Run git clone https://github.com/<YOUR_USERNAME>/babel.git && cd babel
  4. Run yarn && make bootstrap
  5. Wait ⏳
  6. Run make watch (or make build whenever you change a file)
  7. Change the wrong output.js files in the tests/fixture folder of @babel/plugin-transform-flow-strip-types (if there are any)
  8. Update the code!
  9. yarn jest [name-of-the-package-to-test] to run the tests
    • If some test outputs don't match but the new results are correct, you can delete the bad output.js files and run the tests again
  10. If it is working, run make test tu run all the tests
  11. Run git push and open a PR!
@pajaydev
Copy link
Contributor

@nicolo-ribaudo Thanks. I can try this out.

@existentialism
Copy link
Member

@Ajay2507 it's yours! feel free to hit us up if you have any questions!

@pajaydev
Copy link
Contributor

pajaydev commented May 29, 2019

@existentialism Cool. Thanks

@lidoravitan
Copy link
Contributor

@existentialism @pajaydev is there some news with this issue? does still need help? thanks

@pajaydev
Copy link
Contributor

@lidoravitan I have done it, Some test are failing. I need to create a PR and get reviewed.

@lidoravitan
Copy link
Contributor

@pajaydev cool thanks for the response

@pajaydev
Copy link
Contributor

@existentialism @nicolo-ribaudo I created this PR #10120 . this test babel-plugin-transform-flow-strip-types/test/fixtures/regression/class-prop-types is failing, Shall I remove the transform-classes from babel-plugin-transform-flow-strip-types/test/fixtures/regression/class-prop-types/options.json ?.

@pajaydev
Copy link
Contributor

pajaydev commented Jul 2, 2019

@nicolo-ribaudo I fixed the test. Thanks for your help. I appreciate it.

facebook-github-bot pushed a commit to facebook/flow that referenced this issue Feb 27, 2020
Summary:
Flow implemented class fields while they were very early-stage, behind the `esproposal.class_instance_fields` and `esproposal.class_static_fields` flowconfig options.

In current versions of the proposal, fields without initializers now default to `undefined`: [[design](https://github.com/tc39/proposal-class-fields#fields-without-initializers-are-set-to-undefined)] [[spec](https://tc39.es/proposal-class-fields/#sec-define-field)]

Babel < 8 stripped uninitialized fields, which is not spec-compliant. Babel 8 changes to leaving the field and just removing the annotation, as you'd rightly expect, but making it impossible* to have type-only fields. [[babel issue](babel/babel#10039)] [[babel 8 notes](babel/babel#10746)]

This diff introduces a `declare` syntax to provide a solution for type-only fields:

```
class C {
  declare foo: string;
  bar: string;
}

// becomes
class C {
  bar;
}
```

As far as Flow is concerned, both `foo` and `bar` are treated as before, as if they are `string`:

```
const x = new C;
(x.foo: string);
(x.bar: string);
```

Fixes #6811

[* technically you can use Flow comment syntax -- `class C { /*:: prop: string; */ }` but we don't want to require that]

Reviewed By: samwgoldman

Differential Revision: D20088452

fbshipit-source-id: f00299539c9387a55102e9a4e554a3b3bcb3498e
facebook-github-bot pushed a commit to facebook/flow that referenced this issue Mar 3, 2020
Summary:
Flow implemented class fields while they were very early-stage, behind the `esproposal.class_instance_fields` and `esproposal.class_static_fields` flowconfig options.

In current versions of the proposal, fields without initializers now default to `undefined`: [[design](https://github.com/tc39/proposal-class-fields#fields-without-initializers-are-set-to-undefined)] [[spec](https://tc39.es/proposal-class-fields/#sec-define-field)]

Babel < 8 stripped uninitialized fields, which is not spec-compliant. Babel 8 changes to leaving the field and just removing the annotation, as you'd rightly expect, but making it impossible* to have type-only fields. [[babel issue](babel/babel#10039)] [[babel 8 notes](babel/babel#10746)]

This diff introduces a `declare` syntax to provide a solution for type-only fields:

```
class C {
  declare foo: string;
  bar: string;
}

// becomes
class C {
  bar;
}
```

As far as Flow is concerned, both `foo` and `bar` are treated as before, as if they are `string`:

```
const x = new C;
(x.foo: string);
(x.bar: string);
```

Fixes #6811

[* technically you can use Flow comment syntax -- `class C { /*:: prop: string; */ }` but we don't want to require that]

Reviewed By: samwgoldman

Differential Revision: D20088452

fbshipit-source-id: f6102215d5cf13d91c073affd3adf85ce7d97a04
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Mar 10, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
claimed good first issue Has PR i: bug outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants