sprintf.js icon indicating copy to clipboard operation
sprintf.js copied to clipboard

Published 20 hours ago • verified publisher icon alexei

chore: fix package bundling

Open Nytelife26 opened this issue 3 years ago • 10 comments

Nytelife26 avatar Feb 23 '21 17:02 Nytelife26

@alexei I don't know who the other maintainers are so, mentioning you in this here.

Nytelife26 avatar Mar 03 '21 18:03 Nytelife26

What problem does this address?

alexei avatar Mar 04 '21 08:03 alexei

@alexei the fact that package bundling is not done correctly, as the name implies. Currently, the source code is included with bundles, whereas it should be the distribution channel exclusively. This fixes that.

Nytelife26 avatar Mar 04 '21 09:03 Nytelife26

it should be the distribution channel exclusively

Why?

alexei avatar Mar 04 '21 09:03 alexei

@alexei Because that's how package distribution works. You package the metadata and production-ready version of the library. The source code is already available in the repository, and including it adds unnecessary bundle size and slows down dependency installation.

Nytelife26 avatar Mar 04 '21 18:03 Nytelife26

@alexei Because that's how package distribution works.

Hehehe, if that were true, then why is my basic React app using 300 MB of disk space?

The source code is already available in the repository, and including it adds unnecessary bundle size and slows down dependency installation.

  1. If you think the source code should not be included, then I guess main should be updated as well; keeping it beats the argument you're making. From the docs:

Certain files are always included, regardless of settings: ... The file in the "main" field

  1. This PR not only supersedes https://github.com/alexei/sprintf.js/pull/204 but removes the need for a .npmignore file altogether like you mentioned at one point; in this case, then perhaps this PR should drop .npmignore
  2. It's not clear to me whether the files key is a feature of the npm repository, or the npm CLI; if it's the latter, then that complicates things; do you have any docs on this?

alexei avatar Mar 11 '21 10:03 alexei

Hehehe, if that were true, then why is my basic React app using 300 MB of disk space?

I have no idea, because react doesn't use anywhere near that much space. Seems like a you problem. Although, altogether, that is a logical fallacy - how can you disprove a statement using one edge case?

main should be updated as well; keeping it beats the argument you're making.

It can be, yes, I just wasn't sure whether you elected to have it like that in the first place for clarity, and removing it wouldn't pose too much difference.

This PR not only supersedes #204 but removes the need for a .npmignore file altogether

Indeed it should. I'll do that.

It's not clear to me whether the files key is a feature of the npm repository, or the npm CLI

What do you mean by "a feature of the npm repository"? The files directive is part of the package manifest standard and is honoured by all major bundlers.

Nytelife26 avatar Mar 12 '21 22:03 Nytelife26

@alexei Status?

Nytelife26 avatar Apr 14 '21 23:04 Nytelife26

@Nytelife26 sorry - I didn't have time to verify this and I don't consider it urgent anyway

alexei avatar Apr 15 '21 22:04 alexei

I would pick this up if I had an indication my PR would get merged. I agree that removing .npmignore (blacklist) and adding the files that should be published on npm in the code tab should be specified.

I'd be more inclusive and also include CONTRIBUTORS.md, because it's the friendly thing to do. Or, move them into the package.json.

msimerson avatar Apr 05 '24 01:04 msimerson