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

chore: fix package bundling #601

Merged
merged 5 commits into from
Mar 25, 2021
Merged

chore: fix package bundling #601

merged 5 commits into from
Mar 25, 2021

Conversation

Nytelife26
Copy link
Contributor

As of present, undici is very large due to its accidental inclusion of all files in the repository. This would fix that, significantly reducing bundle size in all future releases.

@codecov-io
Copy link

codecov-io commented Mar 20, 2021

Codecov Report

Merging #601 (943d425) into main (b3dd69f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #601   +/-   ##
=======================================
  Coverage   99.10%   99.10%           
=======================================
  Files          17       17           
  Lines        1446     1446           
=======================================
  Hits         1433     1433           
  Misses         13       13           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3dd69f...943d425. Read the comment docs.

package.json Outdated Show resolved Hide resolved
@mcollina
Copy link
Member

I would prefer to keep the tests and docs in the released package.

@Nytelife26
Copy link
Contributor Author

Nytelife26 commented Mar 22, 2021

I would prefer to keep the tests and docs in the released package.

There is no reason for doing so. All docs are available online and the test suite is not important to the end-user (and is additionally available in this repository). Ultimately, I am not the package author nor any of its maintainers, so this isn't my call, but I can justifiably recommend that you only include the distribution channel and its necessities in the distributed package.

After all, the entire purpose of undici is to offer superior performance over native. What is the point of improving in one performance aspect just to end up sacrificing in another?

Could you make this more exact?...

Yes, that's absolutely fine. I'll get on it right away! Although, is there anything in lib that shouldn't be published? I wrote the lib inclusion directive that way to add in the whole directory, since to my knowledge everything in lib is the undici package itself.

@mcollina
Copy link
Member

We are not in agreement. Not shipping docs and tests lower the developer experience.

Anyway, I'm leaving this to the rest of the team to decide.

@mcollina mcollina requested review from ronag and dnlup March 22, 2021 22:31
@Ethan-Arrowood
Copy link
Collaborator

docs should 100% be published for offline access (i've used this extensively in the past when traveling).

tests on the other hand im indifferent about. When I use a dependency I trust that the build published to npm is the one tested via the package's CI/CD environment. If im not mistaken we might have more test code than actual source code so I totally see the argument of omitting them, but I also understand where Matteo is coming from since it fits with my previous argument of offline capabilities as well as inherent trust in a dependency (having test locally means they can be executed locally as well to verify the downloaded version of the package does what I want it to do).


TLDR: We should 100% ship docs. I'd like us to also ship tests but I understand if we don't. We can omit files like the docsify site and the benchmarks.

@Nytelife26
Copy link
Contributor Author

docs should 100% be published for offline access (I've used this extensively in the past when travelling).

I would like to add that if you foresee need the documentation of a package offline, you should retrieve it from its repository alongside the installation, however, this isn't my decision, so I will be including documentation in the bundle unless any other team members object.

having tests locally

Once again, if you so wish to verify that a package has been tested properly, you should probably retrieve the test suite from its repository. I feel obliged to note that this is a package ecosystem - testing is for development.

That, and

more test code than actual source code

Yes, several kilobytes more. I believe actually if I recall correctly from a scan I did with UNPKG earlier, the test code is actually more than twice the size of the actual package.

Copy link
Contributor

@dnlup dnlup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can agree on removing the benchmarks and the site assets, I am sure they are not needed offline.

I have rarely needed to read the docs from an installed package directory, as well as run tests from it unless there was some bug to fix on the spot. For that reason, I usually follow the approach suggested here in my packages too.

That said, that is just my experience and I am sure the others have their reasons for keeping them. So I am not against keeping them unless there is a massive performance penalty to pay and I think that there isn't yet.

@Nytelife26
Copy link
Contributor Author

So, just to confirm, the consensus is to keep documentation but omit testing? I'm personally of the opinion that if you foresee needing to view documentation offline you should download it prior, however that isn't my call, so if we can confirm the consensus I can finalize this, fix merge requests, and mark it off as ready :)

@mcollina
Copy link
Member

From some measurements we did a few years back, the download size of a Node.js module does not matter much in terms of install time (for packages < 1MB compressed). The thing that matters the most is the number of dependencies of the same package as every dependency needs to be resolved before downloading. It's a recursive algorithm of network operations of a potentially long depth: for small modules, the latency of doing the network operation is higher than the time it takes to transfer the package.

The only thing we are optimizing by removing code files is space on disk. I am of the opinion that's pretty cheap these days.

Things that matters a lot for startup time is the amount of files (and code) that are loaded when the module is required/imported.

@Nytelife26
Copy link
Contributor Author

All merge conflicts resolved. Ready for merge.

package.json Show resolved Hide resolved
@dnlup dnlup mentioned this pull request Mar 24, 2021
Copy link
Contributor

@dnlup dnlup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Nytelife26

@mcollina
Copy link
Member

This conflicts now.

@ronag ronag added this to the 4.0 milestone Mar 24, 2021
@ronag
Copy link
Member

ronag commented Mar 25, 2021

@mcollina can we merge this?

@Nytelife26
Copy link
Contributor Author

can we merge this?

It appears to conflict with a recent commit. I will fix it now, and then it should be ready for merge.

@dnlup dnlup mentioned this pull request Mar 25, 2021
7 tasks
@mcollina mcollina merged commit 56bd998 into nodejs:main Mar 25, 2021
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
* chore: fix package bundling

* chore: specify index inclusion

* chore: bundle documentation

* chore: add types
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

Successfully merging this pull request may close these issues.

6 participants