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

[PROPOSAL][AWAITING TS 2.1] Use tsc's --importHelpers option to reduce the code size. #2436

Closed
tetsuharuohzeki opened this issue Mar 3, 2017 · 12 comments

Comments

@tetsuharuohzeki
Copy link
Contributor

  • This is a proposal for the future semver major.
  • After TypeScript 2.1, we can use external helper module to emit a down-level transforming. By this option, we may have a chance to reduce our code size.
@kwonoj
Copy link
Member

kwonoj commented Mar 3, 2017

As far as I know this requires consumer to import tslib for their own, isn't it?

@tetsuharuohzeki
Copy link
Contributor Author

@kwonoj

I seem we need to add tslib into our dependencies, but we don't have to require installing tslib for our user.

@csvn
Copy link
Contributor

csvn commented Jan 24, 2018

In my project when using rollup, I end up with 103 __extends from rxjs alone (__extends$1 => __extends$103). It would be great if this could be considered again. as @saneyuki says, it could simply be added as a dependency for rxjs. Externally it should not be noticed by users.

@csvn
Copy link
Contributor

csvn commented Feb 19, 2018

@benlesh Is this something that can be considered for v6 with the other large restructuring?

With all imports used from rxjs@alpha, the esm5 version is ~32KB larger (~55KB non-minified) than if importHelpers would be used. I made a small example repo here: https:/csvn/rxjs-tslib-example.

As I see it, there are four possible options:

  • Use importHelpers with tslib as peerDependency
  • Use importHelpers with tslib as dependency
  • Use rollup/webpack to combine all files into one file for each entry point with es2015 module exports (e.g. index.esm.js + operators.esm.js)
  • Do nothing, and keep repeated helpers

Using tslib as peerDependency would need to be done in i a major version.

@benlesh
Copy link
Member

benlesh commented Feb 20, 2018

@csvn, I think most people are using rollup/webpack to combine the files. This had fallen off my radar, and I'm not exactly sure what's best practice here now. Let me confer with a few people.

@kwonoj
Copy link
Member

kwonoj commented Feb 20, 2018

I am concerned about making this as dep, as rx uses pinned version of dep it could cause 2 different tslib set (user code have latest tslib version a, while rx still uses old version b). Reason I haven't push forward this in v5. We could make as peerdep and open version instead of pinning, but that opens possibility breaking change in tslib (or incorrect combination between compiler) causes some unexpected behavior.

Also curious what's best practices to resolve those cases. If there is, I'd definitely push it for v6.

@benlesh
Copy link
Member

benlesh commented Feb 20, 2018

cc @jasonaden @IgorMinar

@csvn
Copy link
Contributor

csvn commented Feb 20, 2018

I agree that peerDependency is probably better to use than as a dependency. Another reason it might be useful to include tslib is if any other features requiring helper methods are used, such as rest params, object spread, async/await etc. Thanks for looking into this!

@IgorMinar
Copy link
Contributor

I implemented this feature in #3416. I did the following three things very intentionally (but I'm open to discussion if someone finds them problematic):

  1. importHelpers is enabled only for esm5 and esm2015 builds because these are the builds that are further optimized by other build tools (rollup and webpack).
  2. importHelpers are disabled for cjs builds - we still emit the helpers inline in each file - this was done to preserve the current SystemJS support for folks that still require SystemJS - we can revisit this in the future versions if we decide that we no longer want to support SystemJS. In the meantime , I'm not aware of any downside for node users (primary users of cjs builds) when it comes to inlining the helpers in cjs builds.
  3. I set tslib to be a dependency rather than peerDependency of rxjs - this was done to not require that every application that depends on rxjs has to provide tslib in it's package.json. A change like this would make adoption of RxJS v6 much harder for developers that don't already use tslib in their project (e.g. most non-Angular projects would be affected, while Angular users would be unaffected).
  • the downside of declaring tslib as dependency is that there is a potential for having to deal with duplicate versions of tslib in a single project which could increase the size very slightly but shouldn't have any other impact. tslib is a stateless library, so AFAIK it's ok to have multiple versions of it in a single runtime dependency graph. For users that do care about size, they can just ensure to provide a compatible tslib in the root package.json of the app and let npm/yarn dedupe the dependencies - in most cases this should happen without users having to worry about it.

IgorMinar referenced this issue Mar 14, 2018
…m5 and esm2015 builds (#3416)

I intentionally didn't use the flag for cjs builds because that would
make cjs builds incompatible with SystemJS without further configuration
on the client-side - this would be highly undesirable.
@csvn
Copy link
Contributor

csvn commented Mar 16, 2018

I'd just like to mention, just by upgrading rxja alpha, I got the following saving for my lib:
With [email protected]: 920KB (351KB minified)
With [email protected]: 812KB (340KB minified) (-13% and -3% respectively)

Thanks a lot @IgorMinar and @benlesh! 🎉✨

@csvn
Copy link
Contributor

csvn commented Mar 16, 2018

This issue seems safe to close to me.

@kwonoj
Copy link
Member

kwonoj commented Sep 6, 2018

Yeah, I think this is done.

@kwonoj kwonoj closed this as completed Sep 6, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants