-
Notifications
You must be signed in to change notification settings - Fork 49k
Check in a forked version of object-assign only for UMD builds #18180
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
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit c9ebd39:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slightly related, we have an open issue about including the license banner in prod builds #17492
I'll just take one of our own implementations then. Probably easier than figuring out the banner and saves bytes. |
Details of bundled changes.Comparing: 4ee592e...c9ebd39 react
React: size: -4.6%, gzip: -4.7% Size changes (experimental) |
8b22fbc
to
f727b1d
Compare
This one uses ES modules so that we can inline it into UMD builds. We could wait for object-assign to make an ESM export but we're going to remove this dependency and assume global polyfills in the next version anyway. However, we'd have to figure out how to keep the copyright header and it'll get counted in terms of byte size (even if other tooling removes it). A lot of headache when we have our own implementation anyway. So I'll just use that. Ours is not resilient to checking certain browser bugs but those browsers are mostly unused anyway. (Even FB breaks on them presumably.) We also don't need to be resilient to Symbols since the way React uses it we shouldn't need to copy symbols
f727b1d
to
6bd7b4c
Compare
export default Object.assign || | ||
function(target, sources) { | ||
if (target == null) { | ||
throw new TypeError('Object.assign target cannot be null or undefined'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually care about this? Would we expect to ever happen with our usage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe if it's a bug. Meh. I mostly just took it as-is from FB.
The polyfill needs to be able to feature detect Object.assign.
@@ -28,6 +28,10 @@ module.exports = function autoImporter(babel) { | |||
|
|||
visitor: { | |||
CallExpression: function(path, file) { | |||
if (file.filename.indexOf('object-assign') !== -1) { | |||
// Don't replace Object.assign if we're transforming object-assign | |||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're now transforming the file itself, I needed to ensure this bails out.
Tested fixtured and ensured that it conditionally picks up the native version. |
This one uses ES modules so that we can inline it into UMD builds.
We could wait for object-assign to make an ESM export but we're going to remove this dependency and assume global polyfills in the next version anyway. However, we'd have to figure out how to keep the copyright header and it'll get counted in terms of byte size (even if other tooling removes it).
A lot of headache when we have our own implementation anyway. So I'll just use that.
Ours is not resilient to checking certain browser bugs but those browsers are mostly unused anyway. (Even FB breaks on them presumably.)
We also don't need to be resilient to Symbols since the way React uses it we shouldn't need to copy symbols