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

Add requireDirective to strip-flow-types for use in React preset #5468

Merged
merged 3 commits into from
Aug 4, 2017

Conversation

existentialism
Copy link
Member

@existentialism existentialism commented Mar 15, 2017

Q A
Patch: Bug Fix? N
Major: Breaking Change? Y
Minor: New Feature? Y
Deprecations? N
Spec Compliancy? N
Tests Added/Pass? Y/Y
Fixed Tickets Fixes #4702
License MIT
Doc PR
Dependency Changes

Let's keep the 7.0 fun train rolling!

- Removes flow preset from preset-react, (https://twitter.com/left_pad/status/841744171723587585).

EDIT Another option would be to add a flag to flow-strip-types that will only strip types if a @flow declaration is present, and then preset-react could just pass that flag.

EDIT 2 flow-strip-types now has a requireDirective option, which preset-react uses.

@mention-bot
Copy link

@existentialism, thanks for your PR! By analyzing the history of the files in this pull request, we identified @danez to be a potential reviewer.

@existentialism existentialism added area: react PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release labels Mar 15, 2017
@hzoo
Copy link
Member

hzoo commented Mar 15, 2017

cc @spicyj @samwgoldman

@danez
Copy link
Member

danez commented Mar 18, 2017

Should add this to the migration guide also that flow is not included in react anymore.

@hzoo
Copy link
Member

hzoo commented Mar 18, 2017

I do kinda like @spicyj's suggestion (mentioned in the OP now) of "Another option would be to add a flag to flow-strip-types that will only strip types if a @flow declaration is present, and then preset-react could just pass that flag."

so it's opt-in per file

@existentialism existentialism changed the title Remove flow and add dev option to react preset Add requireDirective to strip-flow-types for use in React preset Jul 21, 2017
@babel babel deleted a comment from codecov bot Jul 21, 2017
@existentialism existentialism force-pushed the react-preset branch 2 times, most recently from e45929b to 10251b9 Compare July 26, 2017 14:20
@loganfsmyth
Copy link
Member

What would people think about throwing an error if the file contains Flow annotations without @flow? This PR currently leaves them in, so people will get errors at runtime, which seems more likely to surprise people.

My motivation for wanting to remove Flow-stripping from preset-react is all about user confusion. If we make them opt in with @flow that seems totally fine to me. It does seem like it should be a build error instead though. Webpack and such will certainly error with this PR already, but if people are building for Node they won't get an error until they run the code which doesn't seem ideal.

@sophiebits
Copy link
Contributor

No objections from the React side and also received positive thoughts from Flow folks on our end. This seems like a good change.

No hard opinion on @loganfsmyth's proposal but I guess erroring early sounds a little nicer. Is there a way to add the syntax-flow plugin only if the file has the directive?

@hzoo
Copy link
Member

hzoo commented Aug 4, 2017

Is there a way to add the syntax-flow plugin only if the file has the directive?

@spicyj that particular change can't really work we can't know anything until it's parse already (so have to parse beforehand, or need to redo how that works which would reparse I think?

@loganfsmyth
Copy link
Member

Yeah, for now instead of if (skipStrip) return; we can do if (skipStrip) throw path.buildCodeFrameError(""); at least.

@existentialism
Copy link
Member Author

@hzoo @spicyj @loganfsmyth updated!

@existentialism existentialism added this to the Babel 7 milestone Aug 4, 2017

## Options

### `development`
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 we can explain here/somewhere that you can use this in another "env" either via "env" config in babelrc (although we didn't deprecate yet), or via babelrc.js

Copy link
Member

@hzoo hzoo left a comment

Choose a reason for hiding this comment

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

nice work!

"babel-plugin-transform-react-display-name": "7.0.0-alpha.18",
"babel-plugin-transform-react-jsx": "7.0.0-alpha.18",
"babel-plugin-transform-react-jsx-self": "7.0.0-alpha.18",
"babel-plugin-transform-react-jsx-source": "7.0.0-alpha.18",
"babel-preset-flow": "7.0.0-alpha.18"
"babel-plugin-transform-react-jsx-source": "7.0.0-alpha.18"
Copy link
Member

Choose a reason for hiding this comment

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

duplicate dep here!

@hzoo hzoo merged commit 8c457e9 into 7.0 Aug 4, 2017
@existentialism existentialism deleted the react-preset branch August 4, 2017 15:32
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: flow area: react outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants