cssinjs / jss

JSS is an authoring tool for CSS which uses JavaScript as a host language.
https://cssinjs.org
MIT License
7.07k stars 397 forks source link

question: it is not clear why @babel/runtime is included in the deps of 10-alpha #975

Closed iamstarkov closed 5 years ago

iamstarkov commented 5 years ago

Two years ago in #397 babel-runtime has been removed by @zerkalica with a reason to reduce end user bundle size, but in august last year in #833 @babel/runtime has been re-added by @TrySound with a readded with the same reasoning of reducing end user bundle size.

I dont understand re-addition of it and I dont claim it is wrong. I perhaps need more details behind this reasoning and how that was meant to work.

iamstarkov commented 5 years ago

I still share my opinion from back in time:

I would argue for removing polyfiills from the repo, as far as every app has its own unique environment every polyfill will be either inefficient or verbose. So i think that polyfilling environment is an app's responsibillity. https://github.com/cssinjs/jss/issues/397#issuecomment-276401726

TrySound commented 5 years ago

@iamstarkov We do not add any polyfills. Current babel runtime allows to share helpers like _extends, _inheris, _createClass between many packages so they are not duplicated dozens of times in user bundle. These helpers also cherry-picked like @babel/runtime/esm/helpers/extends which prevents appearing unnecessary code in user bundles.

NeoLegends commented 5 years ago

Two years ago in #397 babel-runtime has been removed by @zerkalica with a reason to reduce end user bundle size, but in august last year in #833 @babel/runtime has been re-added by @TrySound with a readded with the same reasoning of reducing end user bundle size.

To be fair, I think the reasons provided in #397 were unreasonable in the sense there must have been some issue with tree shaking. The author of the issue claimed that the full 34kb of core-js were imported into his bundle, but in that example only a few helpers / core-js functions were required. With proper tree-shaking applied, the size-penalty would have been much less than that.

After all, the imports from babel-runtime should not be any different than the one babel inlines into the source files (except for loose mode, obviously). This implies that there should, with proper tree shaking, never be a negative (with negative meaning a bigger bundle) difference when extracting helpers into babel-runtime.

I'm very much in favor of extracting the helpers to reduce duplication.

iamstarkov commented 5 years ago

it also means that all consumers need to use webpack 4 and babel 7 to gain anything from @babel/runtime@7 addition

TrySound commented 5 years ago

@iamstarkov Nope. They may use any bundler. It's just esm. Babel is not required at all. User will get smaller bundle since babel runtime helpers are reused across a few dependencies, not just one.

You may even use this bundle in browser

<script>
  window.process = {
    env: {
      NODE_ENV: 'development'
    }
  }
</script>
<script type="module">
  import jss from 'https://unpkg.com/jss@10.0.0-alpha.7/dist/jss.esm.js?module'
  console.log(jss)
</script>
TrySound commented 5 years ago

Look, these imports are not polyfills. They are artefacts of babel transpiling. Just shared runtime for syntax features.

import _extends from '@babel/runtime/helpers/esm/extends';
import _createClass from '@babel/runtime/helpers/esm/createClass';
import _inheritsLoose from '@babel/runtime/helpers/esm/inheritsLoose';
import _assertThisInitialized from '@babel/runtime/helpers/esm/assertThisInitialized';
iamstarkov commented 5 years ago

oh, I see

iamstarkov commented 5 years ago

ok, would you recommend to do the same for jss based ui-kits?

NeoLegends commented 5 years ago

It makes sense to always do this, since it can only be beneficial for your output code size. You have to ensure a properly working build-process with tree-shaking though. In the times of Create-React-App this isn't something to worry about, though.

iamstarkov commented 5 years ago

unless CRA is not sufficient for complex production app