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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ You can also specify some settings that will be shared across all the plugin rul
"createClass": "createReactClass", // Regex for Component Factory to use, default to "createReactClass"
"pragma": "React", // Pragma to use, default to "React"
"version": "15.0" // React version, default to the latest React stable release
"flowVersion": "0.53" // Flow version
},
"propWrapperFunctions": [ "forbidExtraProps" ] // The names of any functions used to wrap the propTypes object, such as `forbidExtraProps`. If this isn't set, any propTypes wrapped in a function will be skipped.
}
Expand Down
2 changes: 1 addition & 1 deletion lib/rules/no-deprecated.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ module.exports = {
return (
deprecated &&
deprecated[method] &&
versionUtil.test(context, deprecated[method][0])
versionUtil.testReactVersion(context, deprecated[method][0])
);
}

Expand Down
6 changes: 3 additions & 3 deletions lib/rules/no-render-return-value.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ module.exports = {
}

let calleeObjectName = /^ReactDOM$/;
if (versionUtil.test(context, '15.0.0')) {
if (versionUtil.testReactVersion(context, '15.0.0')) {
calleeObjectName = /^ReactDOM$/;
} else if (versionUtil.test(context, '0.14.0')) {
} else if (versionUtil.testReactVersion(context, '0.14.0')) {
calleeObjectName = /^React(DOM)?$/;
} else if (versionUtil.test(context, '0.13.0')) {
} else if (versionUtil.testReactVersion(context, '0.13.0')) {
calleeObjectName = /^React$/;
}

Expand Down
2 changes: 1 addition & 1 deletion lib/rules/prefer-stateless-function.js
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ module.exports = {
scope = scope.upper;
}
const isRender = blockNode && blockNode.key && blockNode.key.name === 'render';
const allowNull = versionUtil.test(context, '15.0.0'); // Stateless components can return null since React 15
const allowNull = versionUtil.testReactVersion(context, '15.0.0'); // Stateless components can return null since React 15
const isReturningJSX = utils.isReturningJSX(node, !allowNull);
const isReturningNull = node.argument && (node.argument.value === null || node.argument.value === false);
if (
Expand Down
15 changes: 13 additions & 2 deletions lib/rules/prop-types.js
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 versionUtil = require('../util/version');

// ------------------------------------------------------------------------------
// 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 > 0) {
return true;
}
}
Expand Down Expand Up @@ -857,7 +858,17 @@ module.exports = {
* @returns {ASTNode} The resolved type annotation for the node.
*/
function resolveSuperParameterPropsType(node) {
let annotation = node.superTypeParameters.params[1];
let propsParameterPosition;
try {
// Flow <=0.52 had 3 required TypedParameters of which the second one is the Props.
// Flow >=0.53 has 2 optional TypedParameters of which the first one is the Props.
propsParameterPosition = versionUtil.testFlowVersion(context, '0.53.0') ? 0 : 1;
} catch (e) {
// In case there is no flow version defined, we can safely assume that when there are 3 Props we are dealing with version <= 0.52
propsParameterPosition = node.superTypeParameters.params.length <= 2 ? 0 : 1;
}

let annotation = node.superTypeParameters.params[propsParameterPosition];
while (annotation && (annotation.type === 'TypeAnnotation' || annotation.type === 'NullableTypeAnnotation')) {
annotation = annotation.typeAnnotation;
}
Expand Down
30 changes: 25 additions & 5 deletions lib/util/version.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
/**
* @fileoverview Utility functions for React version configuration
* @fileoverview Utility functions for React and Flow version configuration
* @author Yannick Croissant
*/
'use strict';

function getFromContext(context) {
function getReactVersionFromContext(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.version) {
Expand All @@ -14,8 +14,19 @@ function getFromContext(context) {
return confVer.split('.').map(part => Number(part));
}

function test(context, methodVer) {
const confVer = getFromContext(context);
function getFlowVersionFromContext(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;
} else {
throw 'Could not retrieve flowVersion from settings';
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@jseminck jseminck Aug 22, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

}
confVer = /^[0-9]+\.[0-9]+$/.test(confVer) ? `${confVer}.0` : confVer;
return confVer.split('.').map(part => Number(part));
}

function test(context, methodVer, confVer) {
methodVer = methodVer.split('.').map(part => Number(part));
const higherMajor = methodVer[0] < confVer[0];
const higherMinor = methodVer[0] === confVer[0] && methodVer[1] < confVer[1];
Expand All @@ -24,6 +35,15 @@ function test(context, methodVer) {
return higherMajor || higherMinor || higherOrEqualPatch;
}

function testReactVersion(context, methodVer) {
return test(context, methodVer, getReactVersionFromContext(context));
}

function testFlowVersion(context, methodVer) {
return test(context, methodVer, getFlowVersionFromContext(context));
}

module.exports = {
test: test
testReactVersion,
testFlowVersion
};
Loading