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

[core] Remove Object.assign replacement of simple-assign #3948

Closed
newoga opened this issue Apr 12, 2016 · 4 comments · Fixed by #7219
Closed

[core] Remove Object.assign replacement of simple-assign #3948

newoga opened this issue Apr 12, 2016 · 4 comments · Fixed by #7219
Labels
good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@newoga
Copy link
Contributor

newoga commented Apr 12, 2016

This is a reminder issue to at some point remove babel-plugin-transform-replace-object-assign and simple-assign. I just tested the docs site with my current version of Chrome (49.0.2623.110) and it doesn't seem to have the same style issues we had in the past with Chrome's broken implementation of Object.assign as it related to key ordering.

It's probably a little too early to remove it now since the fix was just recently released and there's a good number of users that still have Chrome 48. But I think we should be able to do it soon.

Related to #3124, #2986

@newoga newoga added the Core label Apr 12, 2016
@newoga newoga changed the title [Core] Remove Object.assign replacement with simple-assign [Core] Remove Object.assign replacement of simple-assign Apr 12, 2016
@owencm
Copy link
Contributor

owencm commented Apr 13, 2016

For what it's worth, unless it's causing excessive maintainability issues I'd recommend keeping the code for Chrome 48 and before for 6 to 18 months since mobile devices in emerging markets especially have a very slow rate of updating native apps. If developers want to use MUI in production in a country like India we'll need to maintain this support for a good amount of time.

@newoga
Copy link
Contributor Author

newoga commented Apr 13, 2016

Thanks for feedback, @owencm! We'll definitely keep that in consideration. I don't think this is a major burden from a maintainability standpoint. I just wanted to document this because the casual onlooker (and even contributor) might not understand what the motivation was behind implementing the transform in the first place.

I know React recently merged a PR (facebook/react#6376) that replaced their own Object.assign implementation with object-assign which uses the browser implementation when available. Though maybe they are not using it in a manner where key order counts.

@newoga
Copy link
Contributor Author

newoga commented May 3, 2016

Just an update on this, we could probably consider using the object-assign ponyfill again because they just merged a change that checks for the "order of keys bug" in the native implementations and overwrites it if it exists.

I suppose the disadvantage is potentially a small one-time runtime penalty of doing the test (inconsequential in my opinion), in exchange for the advantage of using possibly optimized native implementations provided that they are working properly.

This isn't super high priority but I think if we can confirm it's working, it'd be worth swapping out to simplify and reduce our build/build dependencies.

sindresorhus/object-assign#32

@oliviertassinari
Copy link
Member

oliviertassinari commented May 21, 2017

I think that we can remove it on the next branch now. From can-i-use data, the relative usage of Chrome 48 is 0.03%. Also, I don't think that we rely on the key order anymore on the next branch 👍 .

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label May 21, 2017
@oliviertassinari oliviertassinari changed the title [Core] Remove Object.assign replacement of simple-assign [core] Remove Object.assign replacement of simple-assign May 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants