Skip to content

Issue #1309 #1322

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

Jump to bottom
Merged
merged 3 commits into from
Jul 26, 2017
Merged

Issue #1309 #1322

merged 3 commits into from
Jul 26, 2017

Conversation

DianaSuvorova
Copy link
Contributor

Fixes issue #1309

@DianaSuvorova
Copy link
Contributor Author

@ljharb , I am done updating this PR. Thanks 😄

ljharb
ljharb reviewed Jul 24, 2017
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

seems good!

lib/util/Components.js Outdated
@@ -63,14 +63,50 @@ Components.prototype.get = function(node) {
* @param {Object} props Additional properties to add to the component.
*/
Components.prototype.set = function(node, props) {
const popsAreEquivalent = (propA, propB) => {
Copy link
Member

Choose a reason for hiding this comment

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

s/pops/props

can this function be defined at file level, so it doesn't need to be recreated every time set is run?

@ljharb ljharb added the bug label Jul 24, 2017
@ljharb
Copy link
Member

ljharb commented Jul 24, 2017

(also it'd be great to have the PR title explain the change being made, and confine the issue reference to the OP description)

jseminck
jseminck reviewed Jul 25, 2017
View reviewed changes
lib/util/Components.js Outdated
const propsToAdd = [];
newPropList.forEach(newProp => {
let newPropisAlreadyInTheList = false;
propsList.some(prop => {
Copy link
Contributor

@jseminck jseminck Jul 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The use of some() here looks strange. some() returns a boolean but it is not used.

Something like this should work too ( didn't try it though ):

const newPropIsAlreadyInTheList = propsList.some(prop => popsAreEquivalent(prop, newProp))

I think the I in newPropisAlreadyInTheList should be capitalised.

Copy link
Contributor

@jseminck jseminck Jul 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

WDYT of using reduce to write this function?

  const merge = (propsList, newPropList) => {
    return newPropList.reduce((propsList, newProp) => {
      if (propsList.some(prop => popsAreEquivalent(prop, newProp))) {
        return propsList;
      }
      propsList.push(newProp);
      return propsList;
    }, propsList)
  };

jseminck
jseminck reviewed Jul 25, 2017
View reviewed changes
lib/util/Components.js Outdated
return false;
}
return false;
};
Copy link
Contributor

@jseminck jseminck Jul 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I personally find this nested if if else if quite hard to read. It can just be:

    return (
      (propA.name === propB.name) &&
      ((!propA.allNames && !propB.allNames) || (propA.allNames.join('') === propB.allNames.join('')))
    )

Although it's not that simple either since it still has a lot of conditions, I find it a bit easier...: conditionA && ((conditionD && conditionC) || conditionE)

Edit: Also, is it possible that propA has a key allNames but propB does not have this key? In this case, the current logic should crash because it would try to call .join() on an undefined value. I am not sure if this can happen though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it didn't happen yet. But it is a fair concern. I will update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 😄

jseminck
jseminck reviewed Jul 25, 2017
View reviewed changes
lib/util/Components.js Outdated
while (node && !this._list[this._getId(node)]) {
node = node.parent;
}
if (!node) {
return;
}
const id = this._getId(node);
let copyUsedPropTypes;
if (this._list[id]) {
copyUsedPropTypes = this._list[id].usedPropTypes && this._list[id].usedPropTypes.slice();
Copy link
Contributor

@jseminck jseminck Jul 25, 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 possible that copyUsedPropTypes is going to be assigned a value but that it will not be used?

The condition for assigning the value is: if (this._list[id])
The condition for using the value is: if (this._list[id] && props.usedPropTypes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it is possible.

DianaSuvorova
DianaSuvorova commented Jul 25, 2017
View reviewed changes
lib/util/Components.js
const mergeUsedPropTypes = (propsList, newPropsList) => {
const propsToAdd = [];
newPropsList.forEach(newProp => {
const newPropisAlreadyInTheList = propsList.some(prop => usedPropTypesAreEquivalent(prop, newProp));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jseminck , thanks for the review. You are correct about some function. Updated it to be more concise. As for reduce I have only used it for very simple logic. If you can rewrite this fnc, I will be happy to try it and copy paste your code if it works :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this approach is good as well.

lib/util/Components.js
@@ -70,7 +94,16 @@ Components.prototype.set = function(node, props) {
return;
}
const id = this._getId(node);
let copyUsedPropTypes;
if (this._list[id]) {
// usedPropTypes is an array. _extend replaces existing array with a new one which caused issue #1309.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ljharb added a comment here with some explanation for the change. LMK if there is a better place for it.

jseminck
jseminck reviewed Jul 26, 2017
View reviewed changes
npm-debug.log.957467166 Outdated
@@ -0,0 +1,22 @@
0 info it worked if it ends with ok
Copy link
Contributor

Choose a reason for hiding this comment

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

This file was added by accident.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gone. Thanks!

@DianaSuvorova
Copy link
Contributor Author

@ljharb should be all set. thanks :)

@ljharb ljharb merged commit bc5435d into jsx-eslint:master Jul 26, 2017
@dustinsoftware dustinsoftware mentioned this pull request Aug 3, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jseminck jseminck jseminck left review comments

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
bug
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DianaSuvorova @ljharb @jseminck