Skip to content
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

feature:(arrayEquals) fixes 171, search param order shouldn't matter #172

Merged

Conversation

plumpNation
Copy link

@plumpNation plumpNation commented Jun 29, 2023

I modified the arrayEquals function so that we don't sort before doing cheaper condition checks (isArray, length).

Since the search params array will just be an array of strings maybe a sort on both before every item comparison.

In addition I changed every to some so that it stops iterating once it has found a non match.

Fixes #171

@plumpNation plumpNation marked this pull request as ready for review June 30, 2023 07:18
@nutboltu nutboltu self-requested a review July 5, 2023 11:54
}

const sortedA = a.sort();
const sortedB = b.sort();
Copy link
Collaborator

Choose a reason for hiding this comment

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

sort method mutates the array. we can keep the original array unchanged.

const sortedA = a.slice().sort(); // or [...a].sort()

Copy link
Author

Choose a reason for hiding this comment

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

Good point!

@nutboltu
Copy link
Collaborator

nutboltu commented Jul 5, 2023

@plumpNation, thanks for your contribution. I left a comment

@nutboltu
Copy link
Collaborator

nutboltu commented Jul 5, 2023

you might need to fix the prettier as well

@plumpNation
Copy link
Author

Updated the branch, I amended to the original commit. Actually ran lint this time, and added a test for the mutation of the original arrays.

@nutboltu nutboltu merged commit 0cd1196 into linearlabs-workspace:master Jul 6, 2023
2 checks passed
@plumpNation plumpNation deleted the 171-url-params-order branch July 6, 2023 10:55
@plumpNation
Copy link
Author

Thanks for taking this change in so quickly. And many thanks for providing the addon, it's extremely useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

URL params order shouldn't matter
2 participants