Refactor Components into ES6 class syntax #1398
Conversation
lib/util/Components.js
Outdated
| return propsList.concat(propsToAdd); | ||
| } | ||
|
|
||
| _usedPropTypesAreEquivalent(propA, propB) { |
There was a problem hiding this comment.
"_" doesn't make anything private; this change makes them fully public. Let's revert these back to functions so that the available prototype methods don't change.
There was a problem hiding this comment.
Okay, should probably do the same for _getId then?
There was a problem hiding this comment.
Viewed without whitespace, this looks good, except for one comment.
lib/util/Components.js
Outdated
| @@ -670,8 +674,8 @@ function componentRule(rule, context) { | |||
| return updatedRuleInstructions; | |||
| } | |||
|
|
|||
| Components.detect = function(rule) { | |||
| return componentRule.bind(this, rule); | |||
| module.exports = { | |||
There was a problem hiding this comment.
this is broken; you're now only exporting an object with detect, instead of the entire Components constructor.
I think you want module.exports = Object.assign(Components, { detect(rule) { … } });
There was a problem hiding this comment.
I did this on purpose. The way rules interact with this file is:
1/ const Components = require('../util/Components');
2/ create: Components.detect((context, components, utils) => {
No rule is creating a new instance of the Components class directly. Everything related to the Components class happens inside Components.js. It seems like the API for this file could even just be a single function (e.g. detectComponents. Therefore, I don't see the need to export the whole Components constructor. Please correct me if I missed anything regarding this file.
The tests are passing and I ran this branch on a fairly large project and didn't have any issues (though I don't have all the rules enabled, so it's not a solid test).
There was a problem hiding this comment.
new require('eslint-plugin-react/util/Components') would stop working, however, and that's part of the API.
There was a problem hiding this comment.
That's correct! 👍 I would have never thought of that... However, what is the use case for this? I don't see any reason why users would use this plugin in such a way.
But yes, if we would make the change that I proposed here, then in theory we'd have to bump the major version. And we don't want to do that for this PR.
The reason I wanted to remove it is because it was quite confusing for me to understand how this module works, and exporting more than what is needed added to the confusion. If possible, it would be good to get rid of it, but for now I'll revert the change so the API doesn't break.
There was a problem hiding this comment.
I'd say when it's time to make a major bump, this file should just export the detect function directly, if that's what we want.
jseminck
force-pushed
the
components-as-class
branch
from
728987c
to
cf2d6f6
Compare
|
Rebased this. I thought there were some changes to Components class in one PR but had no conflicts. Will still need to double-check! |
While working on #1393 and figuring out how to share code related to prop-types detection based on how
Componentsworks, I noticed this class is still using the old ES5 syntax for defining classes.It seems node 4 supports class syntax, and I think it simplifies things slightly. I also changed the exports of the file to be a single object with a
detect()function, rather than exporting the wholeComponentsclass that also haddetectdefined on it.This is purely refactoring of the structure of the file, I didn't change any logic or comments (though some could probably use an update, e.g.
list()does more than just returning alist()and it's not obvious from the code what exactly is going on).