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

Bundle size #151

Closed
zorn-v opened this issue Jul 5, 2020 · 19 comments
Closed

Bundle size #151

zorn-v opened this issue Jul 5, 2020 · 19 comments

Comments

@zorn-v
Copy link

zorn-v commented Jul 5, 2020

import axios from '@nextcloud/axios'

axios.post('/some/url')
       Asset      Size  Chunks             Chunk Names
ajax-size.js  51.2 KiB       0  [emitted]  ajax-size

vs

import axios from 'axios'

axios.post('/some/url', null, {headers: {requesttoken: OC.requestToken}})
       Asset      Size  Chunks             Chunk Names
ajax-size.js  15.7 KiB       0  [emitted]  ajax-size

35kb minified js code just for setting header... are you serious ?

BTW jQuery.post('/some/url') - size 24 bytes 😄

@ChristophWurst
Copy link
Contributor

35kb minified js code just for setting header... are you serious ?

No, I put this library out as a trap. My intention is to increase everyone's bundles and make their code slow.

BTW jQuery.post('/some/url') - size 24 bytes smile

🍎 and 🟠. You're comparing a bundle with a function call. Very fair. If you're referring to the global $ in Nextcloud then sorry to disappoint but this is deprecated and will be removed as written on so many places.

@ChristophWurst
Copy link
Contributor

Feel free to drop constructive feedback here. Spin up a bundle analyzer and inspect why our abstraction is so much bigger than the original lib.

@zorn-v
Copy link
Author

zorn-v commented Jul 7, 2020

My intention is to increase everyone's bundles and make their code slow.

😃

but this is deprecated and will be removed as written on so many places

Yes I know

Feel free to drop constructive feedback here

Well, I think it is because onRequestTokenUpdate wich in fact something like bloated alternative of addEventListener/dispatchEvent

@zorn-v
Copy link
Author

zorn-v commented Jul 7, 2020

Hmm, the biggest squares is for semver in analyzer. Is it really needed for event-bus ?

@ChristophWurst
Copy link
Contributor

semver in analyzer. Is it really needed for event-bus ?

Is this a trick question again? Of course it's needed, otherwise we'd remove it 🙊

@zorn-v
Copy link
Author

zorn-v commented Jul 7, 2020

I mean REALLY )
Do you agree that 35kb is too much for simple thing that this package do ?
All usage of semver package is here
https://github.com/nextcloud/nextcloud-event-bus/blob/e2ff4f2258c1abcfb9d5fbfb281d1af2fecabdd5/lib/ProxyBus.ts#L13-L20
just for 2 warnings...

@ChristophWurst
Copy link
Contributor

ChristophWurst commented Jul 7, 2020

So what would you suggest? Remove the check, hope for the best and have someone debug it if 💩 hits the fan?

@zorn-v
Copy link
Author

zorn-v commented Jul 7, 2020

Use regexp for example. Or simple split by "." of version string.
Need to check benefits for total size after removing semver dep.

@zorn-v
Copy link
Author

zorn-v commented Jul 7, 2020

Or maybe use another, less size package like https://www.npmjs.com/package/compare-versions

@zorn-v
Copy link
Author

zorn-v commented Jul 7, 2020

Need to check benefits for total size after removing semver dep.

Difference about 7.3Kb

@ChristophWurst
Copy link
Contributor

We should update the event bus here and re-evaluate because of nextcloud-libraries/nextcloud-event-bus#107. The new bundle size might be a lot smaller.

@ChristophWurst
Copy link
Contributor

Let me tag a release so we have 3ba2249 published

@ChristophWurst
Copy link
Contributor

That didn't help much according to https://bundlephobia.com/result?p=@nextcloud/[email protected]. But that also shows that core-js takes the much bigger share of bundle size, the event bus is minor compared to that.

@zorn-v
Copy link
Author

zorn-v commented Jul 8, 2020

But that also shows that core-js takes the much bigger share of bundle size

Is not typescript can compile to specified target (ES5 for example) ?
I mean what a sense of babel and core-js polyfills then ?

@ChristophWurst
Copy link
Contributor

Yes, but just es5. So the apps that use this lib would have to transpile the code again. We use babel and https://www.npmjs.com/package/@nextcloud/browserslist-config instead and pit out something that is already processable by the supported browsers (minus linking the chunks to one bundle).

@zorn-v
Copy link
Author

zorn-v commented Jul 8, 2020

Yes, but just es5... We use babel and https://www.npmjs.com/package/@nextcloud/browserslist-config...

Well, let's compare.

# browserslist-config
  'last 2 versions',
  'ie >= 11',
  'firefox ESR',
  'not dead'

https://caniuse.com/#search=es5
'last 2 versions' - true (even IE)
'ie >= 11' - true (see prev)
'firefox ESR' - true (ESR is 78 now)
not dead - true (no comments)

@ChristophWurst
Copy link
Contributor

Awesome. But in that case I'm wondering why core-js is still inserted in so many place. Shouldn't it just have 0 requires and not be used if the claim is true?

@zorn-v
Copy link
Author

zorn-v commented Jul 8, 2020

Maybe it is just habit caused by harsh past of javascript 😄

I'm wondering why core-js is still inserted in so many place

When using typescript it is definetly strange

@vinicius73
Copy link

The @nextcloud/axios v2 is ~60% smaller than v2

https://bundlephobia.com/package/@nextcloud/[email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants