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

Target es5 #11

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Target es5 #11

wants to merge 5 commits into from

Conversation

steveadams
Copy link

Hi there, this is my first attempt at a contribution. Please feel free to let me know exactly what I can do better if I've done anything wrong!

This is the simplest way I could find to change the target to ES5. Since Object's prototype doesn't have assign in ES5, I've opted to import assignIn from lodash.

The code compiles and the tests pass. There are deprecations and vulnerabilities in the compilation process which I've ignored as they seem out of scope for this issue.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 94.191% when pulling a7af461 on SteveAdams:target-es5 into d53dc9a on SSENSE:master.

@coveralls
Copy link

coveralls commented Apr 17, 2019

Coverage Status

Coverage increased (+0.4%) to 94.191% when pulling 7d147cb on SteveAdams:target-es5 into d53dc9a on SSENSE:master.

@quinnlangille
Copy link
Member

Hey @steveadams, super happy to have your contribution! Since we're already using lodash, this seems totally fine to me. We could also achieve the same result without lodash by using the spread operator but in the case, it doesn't make a huge difference.

Just going to ping @kylefix to see if he has any input, as he was working on a few upgrades for the package recently

@steveadams
Copy link
Author

Ah, good eye. Using the spread operator makes a lot more sense here. Benchmarks suggest lodash will be slower (at least using _.merge vs Object.assign vs spread), and using native syntax is hard to beat. I'll go ahead and fix that.

@steveadams
Copy link
Author

I'm not sure if this PR is still applicable, but let me know what I can do to get it merged if it's still useful. Otherwise feel free to close it!

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.

3 participants