Skip to content

Target es5#11

Open
steveadams wants to merge 5 commits into
SSENSE:masterfrom
steveadams:target-es5
Open

Target es5#11
steveadams wants to merge 5 commits into
SSENSE:masterfrom
steveadams:target-es5

Conversation

@steveadams

Copy link
Copy Markdown

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
Copy Markdown

Coverage Status

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

@coveralls

coveralls commented Apr 17, 2019

Copy link
Copy Markdown

Coverage Status

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

@quinnlangille

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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