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

More consistent Usage of ES6+ Code #5056

Merged
merged 13 commits into from
May 28, 2019

Conversation

DomAmato
Copy link
Contributor

@DomAmato DomAmato commented Mar 19, 2019

Resolves N/A
Impact: minor
Type: style|refactor|chore

Issue

Just uses more consistent ES6+ grammar such as

  • Using includes instead of indexOf
  • Property Shorthand
  • Default parameters

Solution

N/A

Breaking changes

No breaking changes or functional changes

Testing

No new testing, tested on circleci and tests passed

@aldeed aldeed changed the base branch from master to develop March 19, 2019 21:59
@DomAmato
Copy link
Contributor Author

Thanks for changing the branch I forgot about it while I was filling out the PR form

@aldeed
Copy link
Contributor

aldeed commented Mar 19, 2019

Thanks @DomAmato. You will have to merge in latest develop to fix conflicts.

While we do want to move toward using modern patterns, I don't think we're comfortable merging this many changes of functions to arrow functions, which may cause subtle bugs. If any of the functions use this inside them, you have now changed the value of this. And in JS, if there aren't tests covering it, this may not be noticeable until that code is executed. Were you checking for this when making these changes?

You will also need to sign all of your commits while rebasing. See https://github.com/reactioncommerce/reaction-component-library/blob/master/docs/contributing.md#signing-your-commits

@brent-hoover
Copy link
Collaborator

Also from the Mocha docs:

Passing arrow functions (aka "lambdas") to Mocha is discouraged. Lambdas lexically bind this and cannot access the Mocha context.

@DomAmato
Copy link
Contributor Author

Perhaps it would be best if I drop the arrow function changes since sifting through it and troubleshooting it might be more problematic?

@aldeed
Copy link
Contributor

aldeed commented Mar 20, 2019

@DomAmato Yes if you can remove those it will be easier to review and approve. You can feel free to force push on this PR branch if necessary.

@DomAmato DomAmato force-pushed the kale-js-updates branch 3 times, most recently from 6d3dfb3 to cc75d47 Compare March 21, 2019 16:49
@DomAmato
Copy link
Contributor Author

ok i squashed the commits since it was easier to sign it once and removed all the arrow function changes

@DomAmato
Copy link
Contributor Author

DomAmato commented Apr 1, 2019

fixed the merge conflicts that came about since I last updated everything. Let me know if there is anything else I need to do pending approval

@aldeed
Copy link
Contributor

aldeed commented Apr 2, 2019

Thanks @DomAmato. I looked through the changes. All of the changes to the app source code are fine, but some files in .reaction/jsdoc directory were changed. Those files are for the API docs and I don't know for sure if any of the jsdoc templates go through Babel or can use ES6+. I suspect they should remain ES5 pending a larger project to update those.

Can you remove those files from this PR? After that it should be OK to merge.

kale-io and others added 12 commits April 2, 2019 16:23
ECMAScript 7 introduced the includes function for arrays so that bitwise and comparisons to -1 are no longer needed

Signed-off-by: Dom Amato <dominic.a.amato@gmail.com>
ECMAScript 7 introduced the includes function for arrays so that bitwise and comparisons to -1 are no longer needed

Signed-off-by: Dom Amato <dominic.a.amato@gmail.com>
ECMAScript 7 introduced the includes function for arrays so that bitwise and comparisons to -1 are no longer needed

Signed-off-by: Dom Amato <dominic.a.amato@gmail.com>
ECMAScript 7 introduced the includes function for arrays so that bitwise and comparisons to -1 are no longer needed

Signed-off-by: Dom Amato <dominic.a.amato@gmail.com>
ECMAScript 7 introduced the includes function for arrays so that bitwise and comparisons to -1 are no longer needed

Signed-off-by: Dom Amato <dominic.a.amato@gmail.com>
ECMAScript 7 introduced the includes function for arrays so that bitwise and comparisons to -1 are no longer needed

Signed-off-by: Dom Amato <dominic.a.amato@gmail.com>
ECMAScript 7 introduced the includes function for arrays so that bitwise and comparisons to -1 are no longer needed

Signed-off-by: Dom Amato <dominic.a.amato@gmail.com>
ECMAScript 7 introduced the includes function for arrays so that bitwise and comparisons to -1 are no longer needed

Signed-off-by: Dom Amato <dominic.a.amato@gmail.com>
ECMAScript 7 introduced the includes function for arrays so that bitwise and comparisons to -1 are no longer needed

Signed-off-by: Dom Amato <dominic.a.amato@gmail.com>
ECMAScript 7 introduced the includes function for arrays so that bitwise and comparisons to -1 are no longer needed

Signed-off-by: Dom Amato <dominic.a.amato@gmail.com>
ECMAScript 7 introduced the includes function for arrays so that bitwise and comparisons to -1 are no longer needed

Signed-off-by: Dom Amato <dominic.a.amato@gmail.com>
Signed-off-by: Dom Amato <dominic.a.amato@gmail.com>
@DomAmato
Copy link
Contributor Author

DomAmato commented Apr 2, 2019

ok i removed those changes that affected those files

@aldeed
Copy link
Contributor

aldeed commented Apr 4, 2019

Waiting until next week to merge but looks good now.

@cos
Copy link
Contributor

cos commented May 24, 2019

@aldeed, fixed the alignment conflict. Ready for merge again.

@aldeed aldeed merged commit 50db6a6 into reactioncommerce:develop May 28, 2019
@jeffcorpuz jeffcorpuz mentioned this pull request Jul 2, 2019
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.

5 participants