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

chore: replace as many Object.assign with object spread as possible #7627

Merged
merged 3 commits into from
Jan 14, 2019

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Jan 14, 2019

Summary

We use @babel/preset-env now, so this is safe.

Test plan

Green CI

@jeysal
Copy link
Contributor

jeysal commented Jan 14, 2019

IIRC someone at v8 was working on it recently, but the last time I checked object spread was still slower than Object.assign in many cases. Did you notice any differences in performance?

@SimenB
Copy link
Member Author

SimenB commented Jan 14, 2019

It's transpiled to babel's implementation anyways, so doesn't matter: https://github.com/babel/babel/blob/694e3fd8cf321988c0f1d97e59a3df77adabad5b/packages/babel-helpers/src/helpers.js#L387-L405

We could use {loose: true, useBultins: true} at which point it'd be transpiled to Object.assign.

That said, object-spread is faster than Object.assign in newer versions of V8, so whenever we drop node 6, we'll be ready 🙂

https://twitter.com/bmeurer/status/1015105293301280768?lang=en

@jeysal
Copy link
Contributor

jeysal commented Jan 14, 2019

It's transpiled to babel's implementation anyways, so doesn't matter: babel/babel:packages/babel-helpers/src/helpers.js@694e3fd#L387-L405

Oh, for some reason I though object spread was introduced in Node 6, but I looked it up and it's 8+, so you're right, it'll be transpiled.
Anyway, it's definitely awesome for readability 👍

@SimenB
Copy link
Member Author

SimenB commented Jan 14, 2019

Anyway, it's definitely awesome for readability

Yeah! Also easier for type systems (as you can see, I was able to remove quite a few $FlowFixMe comments 🙂)

packages/jest-cli/src/FailedTestsCache.js Show resolved Hide resolved
packages/jest-config/src/normalize.js Outdated Show resolved Hide resolved
@SimenB SimenB merged commit e38e368 into jestjs:master Jan 14, 2019
@SimenB SimenB deleted the object-spread branch January 14, 2019 19:40
captain-yossarian pushed a commit to captain-yossarian/jest that referenced this pull request Jul 18, 2019
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants