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 #205

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

Nytelife26
Copy link

No description provided.

@Nytelife26 Nytelife26 mentioned this pull request Feb 23, 2021
@Nytelife26
Copy link
Author

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

@alexei
Copy link
Owner

alexei commented Mar 4, 2021

What problem does this address?

@Nytelife26
Copy link
Author

@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.

@alexei
Copy link
Owner

alexei commented Mar 4, 2021

it should be the distribution channel exclusively

Why?

@Nytelife26
Copy link
Author

@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.

@alexei
Copy link
Owner

alexei commented Mar 11, 2021

@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 Update .npmignore #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?

@Nytelife26
Copy link
Author

Nytelife26 commented Mar 12, 2021

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
Copy link
Author

@alexei Status?

@alexei
Copy link
Owner

alexei commented Apr 15, 2021

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

@msimerson
Copy link
Contributor

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.

@Nytelife26
Copy link
Author

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.

There have been no updates other than for licensing in the past 3 years. I believe it's safe to assume the project will not be actively maintained, which is fine, because the ecosystem doesn't need it, but also strange, because it has 50 million weekly downloads.

If you have agreed to take over maintenance, or you have administrative capacity to make a change like this by some means, I wouldn't mind finishing the job.

@pkuczynski
Copy link

I can also help here if there is any interest and migrate to typescript

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.

4 participants