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

Don't publish tests or TypeScript source files #148

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

midgleyc
Copy link

Looking at unpkg, you're currently publishing the tests, among other files. I don't think this is necessary.

This PR limits the published files to those in browser, es6, lib, package.json, LICENSE and README.md (the last three are implicit with how files works). I think this should cover everything comment-parser needs to work.

This saves about 180kB, or about half the size of the package.

@syavorsky
Copy link
Owner

Sorry for the late response. it does make sense, but what if someone is already relying on *.ts delivered with the package?
Maybe, moving forward, the better option would be to export different tiny packages like comment-parser-{es6,browser,cjs}

@midgleyc
Copy link
Author

midgleyc commented Jan 6, 2022

I don't think someone can be relying on *.ts files -- you get the types from .d.ts, and I don't think you can import a .ts file inside a .ts file (you could from javascript, but the TypeScript files aren't valid JavaScript). exports in your package.json directs to the es6 and cjs folders, additionally.

The easiest way to find out for sure is to post an update, then check for incoming bug reports.

Splitting the package would be nice, and would mean that users interested in cjs don't need to also download es6 and browser -- even fewer bytes downloaded.

@syavorsky
Copy link
Owner

The easiest way to find out for sure is to post an update, then check for incoming bug reports.

nah, there were a couple of bad episodes when I was breaking other tools dependent on the parser, and I am not feeling confident making any assumptions. I will look into how to automate smaller packages publishing, thanks for suggesting this kind of improvement.

@syavorsky
Copy link
Owner

On other hand, I am not completely sure this is something to be solved in comment-parser. Whoever uses it will do the bundling/tree-shaking/etc the way they need, unless it's some rare pull-in-runtime case

@midgleyc
Copy link
Author

midgleyc commented Jan 6, 2022

On the node side, when I do an npm install I get everything in node_modules, together with all the dependencies those libraries have. If I npm install a library that includes your library, I get all the code you publish, no matter what build steps my direct dependent does.

And when I build a docker container from scratch (or in CI, when running tests) I get those files.

I wouldn't expect these files to make it through to a browser endpoint. But they're going to get pulled down when running test on or building something server-side. And I don't think they need to be.

@syavorsky
Copy link
Owner

I see. It's arguable that 180kB improves the Build/CI time, but baking specific node/browser/es packages may still make sense for someone. I will put it on my stack.

@midgleyc
Copy link
Author

midgleyc commented Jan 6, 2022

Certainly, I don't expect removing files in just this package to be noticeable -- but if all my dependencies are packaging 180kB they don't need to, that quickly adds up.

Thanks for engaging here!

@mcmxcdev
Copy link

I also just noticed through the usage of https://github.com/duniul/clean-modules that comment-parser is still bundling lots of redundant files into end users node_modules folder.

Can we get this PR merged?

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.

3 participants