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

The rxjs package is bloated #3753

Open
sindresorhus opened this issue May 28, 2018 · 25 comments
Open

The rxjs package is bloated #3753

sindresorhus opened this issue May 28, 2018 · 25 comments

Comments

@sindresorhus
Copy link

RxJS version: 6.2.0

The package is more than 6 MB. That is a lot for a utility library.

Why is src included in the package? Source maps and TS definition files are already included for each JS file, so I don't see the need to include the TS source files too.

There's also a lot of bloat related to browser support: bundles, _esm5, _esm2015 (3.8 MB in total). Would you consider publishing a separate package for Node.js usage without all the bloat? For example, rxjs-node.

screen shot 2018-05-28 at 11 40 13

@benlesh
Copy link
Member

benlesh commented May 29, 2018

I agree. 🤷‍♂️

IIRC, src is included because of source map needs. I'm curious as to why you say it's not necessary. The build process is something I've been a little detached from recently.

Most of the bloat is us trying to support everyone's module styles needs without confusing the crap out of users. If we publish rxjs and rxjs-node, then why not rxjs-esm and rxjs-umd and so on? And then all of the examples publicly will be weird and confusing, some will import { fromEvent } from 'rxjs-node'; and others will improt { fromEvent } from 'rxjs';

I'm open to any and all proposed solutions (especially from people that want to help). I know @kwonoj would kill to get this smaller, because they use it with an Electron app over at SlackHQ, and they're including the whole package.

@kwonoj
Copy link
Member

kwonoj commented May 29, 2018

just fyi around Electron side, I made decisions around gave up to deal with node_modules size / loading time and moving toward to bundling (even in main nodejs process, yes) to dodge several cases we needed to handcraft some solutions per each cases.

@csvn
Copy link
Contributor

csvn commented May 29, 2018

@benlesh I suggest to set removeComments to true in tsconfig.base.json. Having comments in d.ts files is enough for e.g. VSCode to show tooltips and more. There is a lot of comments that are used to generate documentation, and removing them from ., _es5 and es2015 would reduce total size by a lot.

@benlesh
Copy link
Member

benlesh commented May 29, 2018

FWIW: There's an opportunity to try to change this for v7, as what I'm looking at doing there is currently in our experimental branch

@kwonoj
Copy link
Member

kwonoj commented May 29, 2018

@csvn's proposal is actually good point, we may able to have as non-breaking changes?

@csvn
Copy link
Contributor

csvn commented May 29, 2018

I made a small test, and then ran npm pack ./dist/package --dry-run. It seems to save 25% size, but not sure about the report from npm.

With comments
image

Without comments
image

@kwonoj
Copy link
Member

kwonoj commented May 29, 2018

@csvn want to send PR for those? 📦

@benlesh benlesh added priority: high AGENDA ITEM Flagged for discussion at core team meetings labels May 29, 2018
@csvn
Copy link
Contributor

csvn commented May 29, 2018

@kwonoj Sure, I'll send a PR in moment

@benlesh
Copy link
Member

benlesh commented May 29, 2018

Another thing worth noting is all of the files left for v5 compat reasons will go away in the next version.

@benlesh
Copy link
Member

benlesh commented May 29, 2018

removeComments: true will actually strip the documentation comments from the .d.ts files as well. That's not desirable. Just tried it locally.

@benlesh
Copy link
Member

benlesh commented May 29, 2018

We can probably figure out a way to strip comments from the src files as well.

@csvn
Copy link
Contributor

csvn commented May 29, 2018

I think that would be harder than it sounds, sourceMappings would have to be re-mapped to the correct lines, wouldn't they?

@benlesh
Copy link
Member

benlesh commented May 29, 2018

We might also look into minifying the different module type outputs. Although I'm not sure any one minifier is particularly good at that.

@csvn
Copy link
Contributor

csvn commented May 29, 2018

But you're right about the comments, I thought TS only removed from source code... but setting it for all configs except tsconfig.cjs.json would probably work? It's the only one that emits declaration files.

@benlesh
Copy link
Member

benlesh commented May 29, 2018

There's really no measure for how much I hate all the different module systems. haha

@kwonoj
Copy link
Member

kwonoj commented May 29, 2018

minifying the different module type outputs

This may need some evalutions, especially size of sourcemap to construct mapped stack trace will sacrifice most of size gain. Also, on node side having sourcemap support is somewhat flaky.

@benlesh
Copy link
Member

benlesh commented May 29, 2018

If we can just get Node to modernize and throw CJS out once and for all, we'd solve this whole problem. :D

@benlesh
Copy link
Member

benlesh commented May 29, 2018

I honestly can't remember why we needed both _esm5 and _esm2015, they seem redundant to me.

@kwonoj
Copy link
Member

kwonoj commented May 29, 2018

as far as I remember esm5 is CJS module only replaces module import to es, while esm2015 compilation target es2015.

@csvn
Copy link
Contributor

csvn commented May 29, 2018

es2015 code is smaller and may be more performant, since e.g. arrow functions and classes aren't transpiled. For those only targeting es2015+ it's pretty nice to be able to use it. Also one important factor may be that native customElement.define() only accepts classes. It throws errors with regular transpiled classes => function constructors.

@csvn
Copy link
Contributor

csvn commented May 29, 2018

Uhm.. the point about custom elements is not really relevant in this project. Ignore that 🙄

@csvn
Copy link
Contributor

csvn commented May 29, 2018

I tried updating the build in #3760, so comments are preserved in d.ts files but removed in all .js files (even .cjs). With the changes in the PR and without legacy-reexports it looks like the size is reduced by 1.51 Mb. At least it's some progress on trying to "remove bloat" 😊

As a side note, removing comments from .cjs should make rxjs slightly faster to require() in node.

@benlesh benlesh removed the AGENDA ITEM Flagged for discussion at core team meetings label Jun 7, 2018
@cpojer
Copy link

cpojer commented Mar 20, 2020

For anyone who doesn't need anything but the CommonJS modules:

7.x branch: react-native-community@65b60e5
6.x branch: react-native-community@ccf0621

You can use a 2 MiB (instead of 20 MiB) version with Yarn with this change in your package.json:

"resolutions": {
  "rxjs": "npm: @react-native-community/rxjs@6.5.4-custom"
}

7.0.0-alpha.1 works as well if you are on the latest edge.

@nikeee
Copy link

nikeee commented Apr 29, 2020

I just checked a fresh install of create-react-app.

These are the stats of the rxjs package (6.5.5):

image

Looking at the directories, I don't really get why the original TypeScript sources (src) are included in the release package. Is there something I am missing? Maybe just the *.js and *.d.ts files would suffice.

The simplest approach to reduce the package size without changing the build process would be an .npmignore file which contains something like this:

*.js.map
*.ts
!*.d.ts

This should save around 2.5MB.

@kwonoj
Copy link
Member

kwonoj commented Apr 29, 2020

Including src is intentional to provide correct code navigations with declarationmap. There are some planned deprecations around esm exports which will cut some of sizes.

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

No branches or pull requests

6 participants