Skip to content

Flow 0.53 support #1377

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Aug 25, 2017
Merged

Flow 0.53 support #1377

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
3e85c55
Attempt at Flow 0.53 support
jseminck Aug 16, 2017
585b94a
Add invalid test case
jseminck Aug 16, 2017
f767f68
Add support and test for custom props name
jseminck Aug 17, 2017
deb3b61
Add invalid test case for custom prop names
jseminck Aug 17, 2017
1a8af56
Revert old implementation and replace by configurable flowVersion
jseminck Aug 18, 2017
d232811
Remove flow version and decide the Props argument location based on t…
jseminck Aug 21, 2017
1ebfcee
Add flowVersion back but without a default. When there is no flow ver…
jseminck Aug 21, 2017
a7957a1
update readme regarding the flowVersion setting
jseminck Aug 21, 2017
18ad186
Fix bug and add tests through helper function
jseminck Aug 21, 2017
bb0a457
Add explicit > check for params length
jseminck Aug 22, 2017
cd96ec9
remove test helper function as we cannot use spread operator
jseminck Aug 22, 2017
6d1b7f1
Add one more test case for the new syntax
jseminck Aug 22, 2017
a6d26d9
Move flowVersion setting in invalid tests closer to the code
jseminck Aug 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions lib/rules/prop-types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const has = require('has');
const Components = require('../util/Components');
const variable = require('../util/variable');
const annotations = require('../util/annotations');
const flowVersionUtil = require('../util/flowVersion');

// ------------------------------------------------------------------------------
// Constants
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

return true;
}
}
Expand Down Expand Up @@ -857,7 +858,13 @@ module.exports = {
* @returns {ASTNode} The resolved type annotation for the node.
*/
function resolveSuperParameterPropsType(node) {
let annotation = node.superTypeParameters.params[1];
let annotation;
if (flowVersionUtil.test(context, '0.53.0')) {
annotation = node.superTypeParameters.params[0];
} else {
annotation = node.superTypeParameters.params[1];
}

while (annotation && (annotation.type === 'TypeAnnotation' || annotation.type === 'NullableTypeAnnotation')) {
annotation = annotation.typeAnnotation;
}
Expand Down
28 changes: 28 additions & 0 deletions lib/util/flowVersion.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* @fileoverview Utility functions for Flow version configuration
*/
'use strict';

function getFromContext(context) {
let confVer = '999.999.999';
// .eslintrc shared settings (http://eslint.org/docs/user-guide/configuring#adding-shared-settings)
if (context.settings.react && context.settings.react.flowVersion) {
confVer = context.settings.react.flowVersion;
}
confVer = /^[0-9]+\.[0-9]+$/.test(confVer) ? `${confVer}.0` : confVer;
return confVer.split('.').map(part => Number(part));
}

function test(context, methodVer) {
const confVer = getFromContext(context);
methodVer = methodVer.split('.').map(part => Number(part));
const higherMajor = methodVer[0] < confVer[0];
const higherMinor = methodVer[0] === confVer[0] && methodVer[1] < confVer[1];
const higherOrEqualPatch = methodVer[0] === confVer[0] && methodVer[1] === confVer[1] && methodVer[2] <= confVer[2];

return higherMajor || higherMinor || higherOrEqualPatch;
}

module.exports = {
test: test
};
81 changes: 81 additions & 0 deletions tests/lib/rules/prop-types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1486,6 +1486,7 @@ ruleTester.run('prop-types', rule, {
' }',
'}'
].join('\n'),
settings: {react: {flowVersion: '0.52.0'}},
Copy link
Collaborator

@EvHaus EvHaus Aug 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@jseminck jseminck Aug 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@yannickcr yannickcr Aug 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

parser: 'babel-eslint'
}, {
code: [
Expand All @@ -1499,6 +1500,7 @@ ruleTester.run('prop-types', rule, {
' }',
'}'
].join('\n'),
settings: {react: {flowVersion: '0.52.0'}},
parser: 'babel-eslint'
}, {
code: [
Expand All @@ -1514,6 +1516,7 @@ ruleTester.run('prop-types', rule, {
' }',
'}'
].join('\n'),
settings: {react: {flowVersion: '0.52.0'}},
parser: 'babel-eslint'
}, {
code: [
Expand All @@ -1534,6 +1537,7 @@ ruleTester.run('prop-types', rule, {
' }',
'}'
].join('\n'),
settings: {react: {flowVersion: '0.52.0'}},
parser: 'babel-eslint'
}, {
code: [
Expand All @@ -1546,6 +1550,7 @@ ruleTester.run('prop-types', rule, {
' }',
'}'
].join('\n'),
settings: {react: {flowVersion: '0.52.0'}},
parser: 'babel-eslint'
}, {
code: [
Expand All @@ -1556,6 +1561,36 @@ ruleTester.run('prop-types', rule, {
' }',
'}'
].join('\n'),
settings: {react: {flowVersion: '0.52.0'}},
parser: 'babel-eslint'
},
{
code: `
type Props = {
foo: string,
};

class Bar extends React.Component<Props> {
render() {
return <div>{this.props.foo}</div>
}
}
`,
settings: {react: {flowVersion: '0.53.0'}},
parser: 'babel-eslint'
}, {
code: `
type FancyProps = {
foo: string,
};

class Bar extends React.Component<FancyProps> {
render() {
return <div>{this.props.foo}</div>
}
}
`,
settings: {react: {flowVersion: '0.53.0'}},
parser: 'babel-eslint'
},
// issue #1288
Expand Down Expand Up @@ -2837,6 +2872,7 @@ ruleTester.run('prop-types', rule, {
column: 35,
type: 'Identifier'
}],
settings: {react: {flowVersion: '0.52.0'}},
parser: 'babel-eslint'
}, {
code: [
Expand All @@ -2856,6 +2892,7 @@ ruleTester.run('prop-types', rule, {
column: 13,
type: 'Property'
}],
settings: {react: {flowVersion: '0.52.0'}},
parser: 'babel-eslint'
}, {
code: [
Expand All @@ -2877,6 +2914,7 @@ ruleTester.run('prop-types', rule, {
column: 7,
type: 'Property'
}],
settings: {react: {flowVersion: '0.52.0'}},
parser: 'babel-eslint'
}, {
code: [
Expand All @@ -2893,6 +2931,7 @@ ruleTester.run('prop-types', rule, {
column: 40,
type: 'Identifier'
}],
settings: {react: {flowVersion: '0.52.0'}},
parser: 'babel-eslint'
}, {
code: [
Expand All @@ -2909,6 +2948,7 @@ ruleTester.run('prop-types', rule, {
column: 42,
type: 'Identifier'
}],
settings: {react: {flowVersion: '0.52.0'}},
parser: 'babel-eslint'
}, {
code: [
Expand All @@ -2927,6 +2967,47 @@ ruleTester.run('prop-types', rule, {
column: 42,
type: 'Identifier'
}],
settings: {react: {flowVersion: '0.52.0'}},
parser: 'babel-eslint'
}, {
code: `
type Props = {
foo: string,
};

class Bar extends React.Component<Props> {
render() {
return <div>{this.props.bar}</div>
}
}
`,
errors: [{
message: '\'bar\' is missing in props validation',
line: 8,
column: 37,
type: 'Identifier'
}],
settings: {react: {flowVersion: '0.53.0'}},
parser: 'babel-eslint'
}, {
code: `
type FancyProps = {
foo: string,
};

class Bar extends React.Component<FancyProps> {
render() {
return <div>{this.props.bar}</div>
}
}
`,
errors: [{
message: '\'bar\' is missing in props validation',
line: 8,
column: 37,
type: 'Identifier'
}],
settings: {react: {flowVersion: '0.53.0'}},
parser: 'babel-eslint'
}
]
Expand Down