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

Remove support for Flow comment types #13687

Merged
merged 3 commits into from
Oct 22, 2022

Conversation

thorn0
Copy link
Member

@thorn0 thorn0 commented Oct 19, 2022

Description

See #13142 (comment)

Checklist

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@thorn0
Copy link
Member Author

thorn0 commented Oct 19, 2022

Something isn't right with babel-flow:

Prettier pr-13687
Playground link

--parser babel-flow

Input:

const c = (data/*: /* this is an object *-/ Object */) => {};

Output:

const c = (data: bject) => {};

@fisker
Copy link
Member

fisker commented Oct 19, 2022

AST shows identifier "bject"

@thorn0
Copy link
Member Author

thorn0 commented Oct 19, 2022

@fisker
Copy link
Member

fisker commented Oct 19, 2022

I thought "removing support for Flow comment types" means treat them as normal comments.

Aren't we breaking code? (Making runnable js code not runnable)

@thorn0
Copy link
Member Author

thorn0 commented Oct 19, 2022

Just don't use the flow and babel-flow parsers if you want to treat them as normal comments.

@fisker
Copy link
Member

fisker commented Oct 20, 2022

Make sense.

@fisker
Copy link
Member

fisker commented Oct 20, 2022

@babel/parser updated #13654

@thorn0
Copy link
Member Author

thorn0 commented Oct 20, 2022

Treating them like normal comments will break things for people who use this syntax though. So I don't know how to proceed.

Prettier pr-13687
Playground link

--parser babel

Input:

const a = (b /*: c */)

Output:

const a = b /*: c */;

@thorn0
Copy link
Member Author

thorn0 commented Oct 20, 2022

An idea for a heuristic: if at least one such comment is found in the input (and parser is flow/babel-flow), print all non-JS constructions as Flow comments.

@thorn0
Copy link
Member Author

thorn0 commented Oct 20, 2022

On the other hand, if we decided to "remove support", it should mean it's okay to break that case. 🤔

@fisker
Copy link
Member

fisker commented Oct 20, 2022

How hermes-eslint handle this?

@thorn0
Copy link
Member Author

thorn0 commented Oct 20, 2022

I know little about Hermes. But the version of it found on AST explorer doesn't support this comment syntax at all: https://astexplorer.net/#/gist/2fe013d7b342a4a087b9a96394b04971/13ef1d3e2afe4088a56859c37faaccb96c7b7a46
Which might mean this syntax is considered kind of deprecated.

@fisker
Copy link
Member

fisker commented Oct 20, 2022

@gkz What do you think about this change?

@pieterv
Copy link
Contributor

pieterv commented Oct 20, 2022

Removing support for this feature from prettier seems reasonable to me.

Which might mean this syntax is considered kind of deprecated.

It roughly is, a lot of our infra already doesn't support it super well (e.g. Flows own flow codemod feature) and it's not so useful now that build steps are so ubiquitous even for node scripts. We will likely officially deprecate it from Flow soon.

How hermes-eslint handle this?

hermes-eslint uses the hermes parser (hermes-parser). This is a WASM compiled version of the parser the HermesVM uses. This parser does not have any special support for Flow comments, so they stay comments (just like if you used any other parser). So they still kinda work depending on the accuracy of the prettier comment attachment logic for each comment location.

As a side note this parser works well with prettier, we use it for writing codemods: https://github.com/facebook/hermes/blob/main/tools/hermes-parser/js/hermes-transform/src/transform/transform.js#L66-L76.

@thorn0 thorn0 marked this pull request as ready for review October 21, 2022 10:18
@fisker
Copy link
Member

fisker commented Oct 21, 2022

Will this /*: break our whitespace/new line detect?

@thorn0
Copy link
Member Author

thorn0 commented Oct 21, 2022

Probably, but not more than it's already possible for the flow parser.

@thorn0 thorn0 linked an issue Oct 21, 2022 that may be closed by this pull request
@thorn0
Copy link
Member Author

thorn0 commented Oct 21, 2022

Found an inconsistency between flow and babel-flow (example from #7510):

Prettier pr-13687
Playground link

--parser flow

Input:

(a /* b */ /*: c */)

Output:

(a: /* b */ c);

Prettier pr-13687
Playground link

--parser babel-flow

Output:

(a /* b */: c);

@fisker
Copy link
Member

fisker commented Oct 21, 2022

Are you going to fix it? Feel it will be difficult. How about just pretend we don't know? 😄

@thorn0
Copy link
Member Author

thorn0 commented Oct 22, 2022

Are you going to fix it? Feel it will be difficult. How about just pretend we don't know? 😄

I'm curious what's going on there, but I'll open a separate issue for it.

@thorn0 thorn0 merged commit e67d8d6 into prettier:next Oct 22, 2022
@thorn0 thorn0 deleted the remove-flow-comment-types branch October 22, 2022 22:00
@thorn0 thorn0 mentioned this pull request Oct 22, 2022
4 tasks
@@ -0,0 +1,27 @@
#### Remove support for Flow comments (#13687 by @thorn0)
Copy link
Member

Choose a reason for hiding this comment

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

@thorn0 This should be marked as [BREAKING]?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, let's do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in #13703

Th3S4mur41 pushed a commit to Th3S4mur41/demo-auto-security-release that referenced this pull request Sep 5, 2023
## [1.0.3](v1.0.2...v1.0.3) (2023-09-05)

### Dependencies and Other Build Updates

* **deps-dev:** bump @commitlint/cli from 17.6.5 to 17.6.6 ([#53](#53)) ([22b8bbe](22b8bbe))
* **deps-dev:** bump @commitlint/cli from 17.6.6 to 17.6.7 ([#59](#59)) ([702df77](702df77))
* **deps-dev:** bump @commitlint/cli from 17.6.7 to 17.7.0 ([#61](#61)) ([043c953](043c953))
* **deps-dev:** bump @commitlint/cli from 17.7.0 to 17.7.1 ([#63](#63)) ([e58c315](e58c315))
* **deps-dev:** bump @commitlint/config-conventional from 17.6.5 to 17.6.6 ([#54](#54)) ([1d99376](1d99376))
* **deps-dev:** bump @commitlint/config-conventional from 17.6.6 to 17.6.7 ([#58](#58)) ([8a99190](8a99190))
* **deps-dev:** bump @commitlint/config-conventional from 17.6.7 to 17.7.0 ([#62](#62)) ([b9e2f9f](b9e2f9f))
* **deps-dev:** bump prettier from 2.8.8 to 3.0.0 ([#57](#57)) ([2bdc7df](2bdc7df)), closes [prettier/prettier#14435](prettier/prettier#14435) [prettier/prettier#14212](prettier/prettier#14212) [prettier/prettier#14391](prettier/prettier#14391) [prettier/prettier#13687](prettier/prettier#13687) [prettier/prettier#13732](prettier/prettier#13732) [prettier/prettier#13731](prettier/prettier#13731) [#15011](https://github.com/Th3S4mur41/demo-auto-security-release/issues/15011) [#15010](https://github.com/Th3S4mur41/demo-auto-security-release/issues/15010) [#15009](https://github.com/Th3S4mur41/demo-auto-security-release/issues/15009) [#14901](https://github.com/Th3S4mur41/demo-auto-security-release/issues/14901) [#14896](https://github.com/Th3S4mur41/demo-auto-security-release/issues/14896) [#14994](https://github.com/Th3S4mur41/demo-auto-security-release/issues/14994) [#14995](https://github.com/Th3S4mur41/demo-auto-security-release/issues/14995) [#15002](https://github.com/Th3S4mur41/demo-auto-security-release/issues/15002) [#15000](https://github.com/Th3S4mur41/demo-auto-security-release/issues/15000)
* **deps-dev:** bump prettier from 3.0.0 to 3.0.1 ([#60](#60)) ([724d609](724d609))
* **deps-dev:** bump prettier from 3.0.1 to 3.0.2 ([#64](#64)) ([9783d95](9783d95))
* **deps-dev:** bump prettier from 3.0.2 to 3.0.3 ([#67](#67)) ([c9a7f2a](c9a7f2a))
* **deps-dev:** bump semantic-release from 21.0.5 to 21.0.6 ([#55](#55)) ([ff8e146](ff8e146))
* **deps-dev:** bump semantic-release from 21.0.6 to 21.0.7 ([#56](#56)) ([b601697](b601697))
* **deps-dev:** bump semantic-release from 21.0.7 to 21.0.9 ([#65](#65)) ([72e37ac](72e37ac))
* **deps-dev:** bump semantic-release from 21.0.9 to 21.1.1 ([#66](#66)) ([d3d00f0](d3d00f0))
* **deps:** bump actions/checkout from 3 to 4 ([#68](#68)) ([14f006f](14f006f)), closes [actions/checkout#1436](actions/checkout#1436) [actions/checkout#1067](actions/checkout#1067) [actions/checkout#1447](actions/checkout#1447) [actions/checkout#1436](actions/checkout#1436) [actions/checkout#1067](actions/checkout#1067) [actions/checkout#1377](actions/checkout#1377) [actions/checkout#579](actions/checkout#579) [actions/checkout#1437](actions/checkout#1437) [actions/checkout#579](actions/checkout#579) [actions/checkout#1437](actions/checkout#1437) [actions/checkout#1196](actions/checkout#1196) [actions/checkout#1287](actions/checkout#1287) [actions/checkout#1369](actions/checkout#1369) [actions/checkout#1376](actions/checkout#1376) [actions/checkout#1196](actions/checkout#1196) [actions/checkout#1287](actions/checkout#1287) [actions/checkout#1369](actions/checkout#1369) [actions/checkout#1289](actions/checkout#1289) [#1286](https://github.com/Th3S4mur41/demo-auto-security-release/issues/1286) [actions/checkout#1246](actions/checkout#1246) [actions/checkout#1246](actions/checkout#1246) [#770](https://github.com/Th3S4mur41/demo-auto-security-release/issues/770) [actions/checkout#1057](actions/checkout#1057) [#1447](https://github.com/Th3S4mur41/demo-auto-security-release/issues/1447) [#1067](https://github.com/Th3S4mur41/demo-auto-security-release/issues/1067) [#1436](https://github.com/Th3S4mur41/demo-auto-security-release/issues/1436)
medikoo pushed a commit to medikoo/prettier-elastic that referenced this pull request Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Print comment type includes as comments
4 participants