Flow 0.53 support #1377

jseminck · 2017-08-16T19:38:51Z

Attempted to add Flow 0.53 support: #1376

I'll have to admit that I am not very familiar with Flow, and am not fully sure if this covers all the use-cases... but this seemed like a rather urgent/important issue so perhaps it can help someone else to get to the correct solution.

But other than that, it does fix the example issue that was failing previously, so there is that 😄

Fixed #453.

ljharb

Seems legit to me but i don't use flow

EvHaus · 2017-08-16T21:43:39Z

lib/rules/prop-types.js

@@ -917,6 +917,12 @@ module.exports = {
        }
      },

+      TypeParameterInstantiation: function(node) {
+        if (node.params && node.params[0].type === 'GenericTypeAnnotation' && node.params[0].id.name === 'Props') {


It would be best not to check for the type name here in case people want to pass in a type with a different name, eg:

type FancyPropsType = { ... } class Bar extends React.Component<FancyPropsType> { }

In fact, it would be great to add that as a unit test to ensure it is supported.

good call; this definitely should be supported.

That makes sense.

But what about this test case that we have. Is that still valid? I suppose not, as now we are passing void for Props and { person: Person } as State ? What was the third parameter?

type Person = { firstname: string }; class Hello extends React.Component<void, { person: Person }, void> { render () { return <div>Hello {this.props.person.lastname}</div>; } }

the third might be context

jseminck · 2017-08-17T17:23:33Z

Actually, the current code supports this syntax but Props has to be the second argument. Look at:

https://github.com/yannickcr/eslint-plugin-react/blob/master/lib/rules/prop-types.js#L860

I suggest, we can make the argument location configurable? I suppose in the past, the second TypedParameter represented Props. Now the first one represent Props, and the second represents State.

Or can we just hard-code it and only support the latest Flow version?

All other changes from this PR should be able to be reverted by just switching let annotation = node.superTypeParameters.params[1]; to let annotation = node.superTypeParameters.params[0]; and then adapting the relevant test-cases where Props used to be the second argument.

WDYT?

ljharb · 2017-08-17T17:51:06Z

Hm, that's a pretty major breaking change - are we sure the order was flipped?

jseminck · 2017-08-17T17:59:17Z

According to https://medium.com/flow-type/even-better-support-for-react-in-flow-25b0a3485627 - Props is definitely the first argument. See quote below, it's a breaking change in Flow.

The biggest change we are making is to modify how you define React class components. From version 0.53.0 and on, the React.Component class will take two type arguments, Props and State (as opposed to the three type arguments including DefaultProps that React.Component took before).

The other code got added in: #1236 - It does not look like there was a corresponding issue.

There is a similar PR (that is still open, I think we can close it?): #1138
Edit actually this PR contains very useful information about how it used to work.

The doc links from the first PR (the one that got merged) now lead to docs that explain that Props is the first argument as well: https://flow.org/en/docs/react/components/

EvHaus · 2017-08-17T18:48:56Z

Yeah, I can confirm that it is indeed a breaking change for the React.Component<> type. Not sure how we want to handle that on the eslint-plugin-react side given that different users might be on different versions of Flow...

ljharb · 2017-08-17T21:21:45Z

I think we'll need a flow version setting just like we have for React.

jseminck · 2017-08-18T05:07:24Z

Great. I didn't know we have that. Makes sense, for rules like no-deprecated. Added a new config option called "flowVersion". Unfortunately, the react version configuration is just called "version".

Replaced the original implementation with one that takes the Flow version in mind. 0.52 checks second argument. 0.53 checks first argument.

Will add some more test cases for 0.53 (probably should duplicate all the test cases we already have?) and update docs. There's also still some shared code between "version" and "flowVersion".

EvHaus · 2017-08-18T05:50:41Z

tests/lib/rules/prop-types.js

@@ -1486,6 +1486,7 @@ ruleTester.run('prop-types', rule, {
        '  }',
        '}'
      ].join('\n'),
+      settings: {react: {flowVersion: '0.52.0'}},


Is it better to have {flowVersion: '0.52.0'} be the default so the eslint-plugin-react upgrade is less likely to break things for people and not force them to have to add this?

Or, is it better to make 0.53.0 the default to encourage upgrades and easy adoption for new users?

Or, should we leave as is -- where there is no default and users have to make a conscious choice which they want to pick?

Any advice here @ljharb?

I suppose it depends on the next eslint-plugin-react version? Major version bump means we can make the default to 0.53. Minor version bump means we need to stick to supporting 0.52 as the default (and allow users to configure to 0.53)?

BTW, the current default for React (and also in this PR for Flow) is 999.999.999. So it just assumes by default "the latest version" if the setting is not specifically set.

To be coherent with the React version we should default to the latest supported version.

But currently a breaking change (or deprecation) in React means a new major version bump for the plugin. Are breaking changes a common thing in Flow ? (I'm afraid this will force us to bump the major version too often).

I've been using Flow since around version 0.40 (about 3-4 months) and this is the first breaking change I've encountered so far. I wouldn't say that breaking changes like this are very common.

Since we've checked this syntax in 0.52, it must be a breaking change to default to 0.53.

My strong preference is to initially release a semver-minor with the default as 0.52, and then later release a semver-major that bumps the default to Flow latest.

…he number of TypedArguments

jseminck · 2017-08-21T15:44:58Z

Updated the PR according to the comment from James Kyle: #1376 (comment)

if there are 3 TypedArguments then we can safely assume the second argument is the Props. If there are 1 or 2, then the first TypedArgument is the prop.

This makes the code work for both old and new versions of Flow!

I'm guessing the flowVersion may come back at some point in the future, but for now we should be safe.

What do you think about this approach?

ljharb · 2017-08-21T15:49:26Z

I was not aware it was legal to omit one of the three types in prior Flow versions; given that, then I think we can safely up the default flowVersion (and we should still keep that setting for future changes)

jseminck · 2017-08-21T15:59:20Z

I think there is some confusion now...

It was not legal to omit one of the three types in prior flow versions. So we can safely assume that with 3 arguments, the user is using older version of Flow and therefore the Props will be the second argument.

For the new version, it is legal to omit one of the two types. Thus, we can safely assume if there is one or two arguments, that the user is using Flow 0.53 and therefore the Props is the first argument.

At least, that's what I understood from the post. Please correct me if I am wrong.

But not sure... maybe the flowVersion approach is safer for the long term. The current code needs comments to even explain what is going on and that is perhaps not a good sign?

ljharb · 2017-08-21T16:10:05Z

I think we're on the same page; this won't be a breaking change.

I think we should absolutely add flowVersion; it should not default to anything; and when flowVersion is absent only, it should use the fallback logic that you describe.

In the future, if we want to add Flow-version-specific behavior, we'll already have a documented setting to use for it.

jseminck · 2017-08-21T16:40:47Z

Aha, yes, I agree that's a good idea! 🎉

We'll have to duplicate a ton of tests though as we'll need to test with both flowVersion configured and without flowVersion configured.

…sion, we detect the props argument position by the arguments length'

ljharb · 2017-08-21T22:16:16Z

lib/rules/prop-types.js

@@ -173,7 +174,7 @@ module.exports = {
     */
    function isSuperTypeParameterPropsDeclaration(node) {
      if (node && node.type === 'ClassDeclaration') {
-        if (node.superTypeParameters && node.superTypeParameters.params.length >= 2) {
+        if (node.superTypeParameters && node.superTypeParameters.params.length) {


please use >= 0 here rather than relying on implicit truthiness checks

ljharb · 2017-08-21T22:16:47Z

lib/util/version.js

+  if (context.settings.react && context.settings.react.flowVersion) {
+    confVer = context.settings.react.flowVersion;
+  } else {
+    throw 'Could not retrieve flowVersion from settings';


doesn't this mean that if it's not specified, it throws?

Yes. That's what we wanted, right?

We don't want to provide a default value, so test() cannot return true or false in case it is not specified. Therefore, throwing an exception instead which we then handle in the calling code by catching it and using the default logic.

Sorry if I wasn't clear; when no version is specified, I want the logic to be "guess which flowVersion it is" - specifying a flow version can't be required, or else it's a very large breaking change.

We are guessing what the flowVersion is, but only for the specific implementation in the prop-types rule: https://github.com/yannickcr/eslint-plugin-react/pull/1377/files#diff-531a13f7edd3ed4e7c65c3a7df7052f0R868

As this is a general utility function, I don't think that here we can easily guess what flow version we are using, because we need more information about the code. In the prop-types example, we need to know how many TypedArguments there are. But if there are none, then we cannot make a guess. If another rule would want to use this utility function, they would have to specify their own criteria/logic for guessing the correct flowVersion.

I can move this utility function into the prop-types rule directly, as it is currently the only place where it is used. I just thought that for future cases it might be good to have it extracted already, so that it can be used in other rules as well.

Ah, i see what you mean.

I guess that's OK, but i feel like maybe returning null here would be easier to deal with than try/catch.

EvHaus

LGTM

ljharb

I'd still prefer if testFlowVersion returned null when absent instead of specifying (deferring to individual rules to throw if the version is ever required), but that's an internal implementation detail so we can tweak it later.

jseminck · 2017-08-22T16:49:12Z

I can still change it.

I prefer the try/catch slightly more because with a null return, if you do if (testFlowVersion(context, "myVersion")), this would be treated as false., That is incorrect, since that would mean we default to the lowest version possible). I think people are more likely to use the function this way as it is not obvious that it can return true, false andnull.

With the throw it would actually throw an error for the developer that is calling the function, and they have to work around it. Until we decide to specify a default value for flowVersion in the future.

Additionally, with a null return the code would also be a bit more complicated (imho) and introduce another variable on which we have to do a null check:

let argumentPosition;
const isFlowVersion53 = testFlowVersion(context, "0.53.0");

if (isFlowVersion53 === null) {
  // use default logic 
} else {
  argumentPosition = isFlowVersion53 ? 0 ; 1
}

But in the grand scheme of things it's just a minor change and I can live with both implementations 😄

jseminck · 2017-08-22T17:11:55Z

A totally different question:

no-unused-prop-types doesn't have any tests for the React.Component<void, Props, void> syntax. Which is surprising... it doesn't seem like it even supports this syntax, and therefore also not the new syntax.

The same for forbid-prop-types and default-props-match-prop-types. Adding support for this syntax for these rules should be out of scope for this PR, but we should probably add it at some point?

I think also that the "props extraction logic" should be extracted to a separate file (like we have Components.js) so that this code can be re-used between these rules.

ljharb · 2017-08-22T17:52:32Z

Yes, it would be ideal if every rule supported this syntax; a separate PR is fine.

If there's logic that can be shared, let's put it in a separate file in this PR, in anticipation.

jseminck · 2017-08-22T18:23:22Z

I'm sure there is logic that can be shared but I think it's too early to tackle it. We would have to figure out what exactly is already duplicated between these two rules and they are both quite big/complicated.

I just stumbled upon this while looking at another bug that was just reported and noticed there is some duplication already. Extracting it will take time...

ZhangYiJiang · 2017-08-30T02:00:56Z

Hi, can I check when this changeset will be released? Will it be the next minor or major release?

willdurand · 2017-10-18T15:55:01Z

@jseminck for some reasons, eslint fails to validate the props (react/prop-types) with the following syntax:

type Props = {|
  className?: string,
  width?: number,
  minWidth: number,
  range: number,
|};

export default class LoadingText extends React.Component<Props> {
}

but this works:

type Props = {|
  className?: string,
  width?: number,
  minWidth: number,
  range: number,
|};

export default class LoadingText extends React.Component<Props, void> {
}

Any idea?

ljharb · 2017-10-18T16:08:47Z

@willdurand please file a new issue

willdurand · 2017-10-18T16:10:24Z

sure thing!

AndrewSouthpaw · 2017-11-18T22:40:38Z

A tip for people upgrading and it doesn't seem to work. If you're working in an IDE (e.g. IntelliJ), make sure you restart it after upgrading your dependencies. IntelliJ does something with caching so the new rules won't be used until after a restart. Everything is working fine afterward. Another hour lost to the ether... 🙄

Thanks @jseminck et. al for the hard work to make this happen!

Attempt at Flow 0.53 support

3e85c55

jseminck changed the title ~~Attempt at Flow 0.53 support~~ WIP - Attempt at Flow 0.53 support Aug 16, 2017

Add invalid test case

585b94a

ljharb approved these changes Aug 16, 2017

View reviewed changes

ljharb added the flow label Aug 16, 2017

ljharb requested review from yannickcr, lencioni and EvHaus August 16, 2017 21:24

EvHaus requested changes Aug 17, 2017

View reviewed changes

jseminck added 2 commits August 17, 2017 20:16

Add support and test for custom props name

f767f68

Add invalid test case for custom prop names

deb3b61

Revert old implementation and replace by configurable flowVersion

1a8af56

EvHaus reviewed Aug 18, 2017

View reviewed changes

Remove flow version and decide the Props argument location based on t…

d232811

…he number of TypedArguments

jseminck added 3 commits August 21, 2017 21:47

Add flowVersion back but without a default. When there is no flow ver…

1ebfcee

…sion, we detect the props argument position by the arguments length'

update readme regarding the flowVersion setting

a7957a1

Fix bug and add tests through helper function

18ad186

ljharb reviewed Aug 21, 2017

View reviewed changes

Add explicit > check for params length

bb0a457

jseminck added 3 commits August 22, 2017 07:49

remove test helper function as we cannot use spread operator

cd96ec9

Add one more test case for the new syntax

6d1b7f1

Move flowVersion setting in invalid tests closer to the code

a6d26d9

jseminck changed the title ~~WIP - Attempt at Flow 0.53 support~~ Flow 0.53 support Aug 22, 2017

EvHaus approved these changes Aug 22, 2017

View reviewed changes

ljharb approved these changes Aug 22, 2017

View reviewed changes

yannickcr approved these changes Aug 24, 2017

View reviewed changes

ljharb merged commit e2f6460 into jsx-eslint:master Aug 25, 2017

ZhangYiJiang mentioned this pull request Aug 28, 2017

Upgrade Flow to v0.54 nusmodifications/nusmods#440

Merged

3 tasks

MoOx mentioned this pull request Sep 5, 2017

Add further support for Flow prop types. #1138

Closed

jseminck mentioned this pull request Sep 5, 2017

Add support for Flow TypedArgument in no-unsed-prop-types rule #1412

Merged

jseminck deleted the flow53 branch November 19, 2017 07:53

ljharb mentioned this pull request Jan 12, 2018

Nested proptypes in FlowType causes RangeError: Maximum call stack size exceeded #913

Closed

jetpacmonkey mentioned this pull request Jan 22, 2018

Support recursive type annotations, fixes #913 #1653

Merged

jomasti mentioned this pull request Jan 5, 2019

Support Flow Prop/State syntax #453

Closed

Flow 0.53 support #1377

Flow 0.53 support #1377

Conversation

jseminck commented Aug 16, 2017 • edited by ljharb Loading

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseminck Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseminck commented Aug 17, 2017 • edited Loading

ljharb commented Aug 17, 2017

jseminck commented Aug 17, 2017 • edited Loading

EvHaus commented Aug 17, 2017

ljharb commented Aug 17, 2017

jseminck commented Aug 18, 2017

EvHaus Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

jseminck Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

yannickcr Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseminck commented Aug 21, 2017 • edited Loading

ljharb commented Aug 21, 2017

jseminck commented Aug 21, 2017 • edited Loading

ljharb commented Aug 21, 2017

jseminck commented Aug 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseminck Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvHaus left a comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

jseminck commented Aug 22, 2017 • edited Loading

jseminck commented Aug 22, 2017 • edited Loading

ljharb commented Aug 22, 2017

jseminck commented Aug 22, 2017

ZhangYiJiang commented Aug 30, 2017

willdurand commented Oct 18, 2017

ljharb commented Oct 18, 2017

willdurand commented Oct 18, 2017

AndrewSouthpaw commented Nov 18, 2017

jseminck commented Aug 16, 2017 •

edited by ljharb

Loading

jseminck Aug 17, 2017 •

edited

Loading

jseminck commented Aug 17, 2017 •

edited

Loading

jseminck commented Aug 17, 2017 •

edited

Loading

EvHaus Aug 18, 2017 •

edited

Loading

jseminck Aug 18, 2017 •

edited

Loading

yannickcr Aug 20, 2017 •

edited

Loading

jseminck commented Aug 21, 2017 •

edited

Loading

jseminck commented Aug 21, 2017 •

edited

Loading

jseminck Aug 22, 2017 •

edited

Loading

jseminck commented Aug 22, 2017 •

edited

Loading

jseminck commented Aug 22, 2017 •

edited

Loading