perf: Save 3740 bytes gizpped by getting rid of xhr deps #6164
Conversation
| subClass.super_ = superClass; | ||
| } | ||
| }; | ||
| import _inherits from '@babel/runtime/helpers/inherits'; |
There was a problem hiding this comment.
_inherits from babel is exactly the same as our code.
There was a problem hiding this comment.
Since what we have is working, why change it?
There was a problem hiding this comment.
Since babel includes the _inherits from @babel/runtime/helpers/inherits in our code regardless, we may as well not include it twice
There was a problem hiding this comment.
This change broke a plugin #6328.
|
I guess trim is used for IE8 support. I wonder if they have a timeline for xhr@3, which should drop obsolete browser support. Made some comments on your xhr commit. Also, we should make sure that regenerator and its runtime aren't included as part of the runtime and external helpers stuff. |
| subClass.super_ = superClass; | ||
| } | ||
| }; | ||
| import _inherits from '@babel/runtime/helpers/inherits'; |
There was a problem hiding this comment.
Since what we have is working, why change it?
| 'aes-decrypter', | ||
| 'keycode' | ||
| ]), | ||
| module(id) { |
There was a problem hiding this comment.
https://rollupjs.org/guide/en/#external (ie it can be a function or a list)
There was a problem hiding this comment.
Ah, I was specifically looking for module rather than external.
package.json
Outdated
| }, | ||
| "devDependencies": { | ||
| "@babel/cli": "^7.4.4", | ||
| "@babel/core": "^7.4.5", | ||
| "@babel/node": "^7.4.5", | ||
| "@babel/plugin-transform-runtime": "^7.4.4", | ||
| "@babel/plugin-external-helpers": "^7.2.0", |
There was a problem hiding this comment.
Last time I played with it, using the external helpers actually made the file size much bigger, so, we should be careful with it.
There was a problem hiding this comment.
I will do some file size tests without it, and just the xhr change.
| 'keycode' | ||
| ]), | ||
| module(id) { | ||
| const result = moduleExternals.some((ext) => id.indexOf(ext) !== -1); |
There was a problem hiding this comment.
would id.startsWith(ext) make sense here instead?
There was a problem hiding this comment.
fyi I changed this because @babel/runtime includes any number of helpers and we don't want to have to list them all :)
It seems like with helpers is fine especially when we take into account that |
|
Cool, those differences are much smaller than last time I tried them out, and we still end up with a smaller output. Yeah, updating those to have better es builds would give us even more savings. |
brandonocasey
force-pushed
the
perf/external-helpers-and-xhr
branch
from
de71080
to
47d8f10
Compare
gkatsev
added
the
minor
brandonocasey
force-pushed
the
perf/external-helpers-and-xhr
branch
from
47d8f10
to
a696b70
Compare
brandonocasey
force-pushed
the
perf/external-helpers-and-xhr
branch
from
c362615
to
84b1af3
Compare
brandonocasey
force-pushed
the
perf/external-helpers-and-xhr
branch
from
84b1af3
to
3e9ac89
Compare
|
Final size versus master (which just saw a size decrease from a video.js change): |
brandonocasey
changed the title
es-abstract is huge, and there isn't really a reason for us to include it. See the changes here:
EDIT: made an actual pull request. videojs/xhr#1
└─┬ xhr@2.5.0
└─┬ parse-headers@2.0.2
└─┬ string.prototype.trim@1.2.0
└── es-abstract@1.13.0 deduped
We also are not using external helpers correctly, this pr fixes that as well.