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

Fix package size by excluding docs #5643

Merged
merged 3 commits into from Oct 26, 2021
Merged

Fix package size by excluding docs #5643

merged 3 commits into from Oct 26, 2021

Conversation

XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Oct 23, 2021

Before:
total files: 572
package size: 260.0 kB
unpacked size: 1.4 MB

After:
total files: 356
package size: 149.1 kB
unpacked size: 780.9 kB

Basically, got rid of all docs and rules README.md files. Now, I know that it might have been intentional, but it makes a big difference. Also, I checked eslint and they don't include the docs either. After all, all this info is available in the repo and the website.

IMHO, at the end of the day, it's probably a good change that will help with the package size and/or install time.

@XhmikosR XhmikosR marked this pull request as ready for review October 23, 2021 06:18
Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

These files are used to generate website :)

@XhmikosR
Copy link
Member Author

XhmikosR commented Oct 23, 2021

And what's the issue exactly? Why do they need to be in the npm package?

They are still in the repo. If they are being used somewhere else as input for the website, then we should change how the website is built. The docs don't need to be in the npm package for the majority of the package consumers, I don't see why we need them to pay the "penalty" whatever that is.

@hudochenkov
Copy link
Member

I see only one issue. This PR should not be merged until our website relies on these files.

@ybiquitous
Copy link
Member

Now, stylelint/stylelint.io installs stylelint directly from GitHub, not from the npm registry, so it seems unnecessary to change the website. 🤔

https://github.com/stylelint/stylelint.io/blob/4949dfb588f2c44418fbbb58550f5a794a28a9f8/package.json#L132

And, I agree with @XhmikosR. 👍🏼
For example, npm diff would be more useful to view real changes between versions:

# too many docs changes... 😓 
npm diff --diff stylelint@13.13.1 --diff=stylelint@14.0.0

@hudochenkov
Copy link
Member

Hm, that's new. The change was done recently and without a PR. @jeddy3 could you share some context?

@hudochenkov
Copy link
Member

I think the idea of using Stylelint from npm is to have website to show docs for that latest released Stylelint version. E. g. if using repository as source of docs, website many times would provide info about unreleased options, rules, etc.

We could use Git tags (instead of main branch) to get docs to website from this repository.

@XhmikosR
Copy link
Member Author

XhmikosR commented Oct 24, 2021 via email

@hudochenkov
Copy link
Member

@XhmikosR I'm in favor of removing docs from the package.

I want to be sure we figure out how website will get content. And stick to it, before we remove docs from the package.

@jeddy3
Copy link
Member

jeddy3 commented Oct 25, 2021

The change was done recently and without a PR. @jeddy3 could you share some context?

Hotfix to make the images on the homepage work after renaming master to main.

We could use Git tags (instead of main branch) to get docs to website from this repository.

It seems that the "files" property is respected when installing from GitHub, and so the docs aren't available to use from this branch.

I want to be sure we figure out how website will get content. And stick to it, before we remove docs from the package.

I agree. We included the docs in the package originally, because it was the simplest way. What's the best, and ideally still simple, way to make all the markdown files available to the website (and be able to fix it to a specific version)?

@ybiquitous
Copy link
Member

How about installing a tarball like this?

{
  "dependencies": {
    "stylelint": "https://github.com/stylelint/stylelint/tarball/14.0.0"
  }
}

See https://docs.npmjs.com/cli/v7/commands/npm-install

image

@hudochenkov
Copy link
Member

Tarball seems a good solution. git clone also might work :)

@jeddy3 jeddy3 changed the title Reduce the number of files included in the npm package Fix package size by excluding docs Oct 25, 2021
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

Tarball works for me.

I'll update the stylelint.io README next release.

PR LGTM.

@jeddy3 jeddy3 mentioned this pull request Oct 25, 2021
6 tasks
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

LGTM! 👍🏼

@XhmikosR
Copy link
Member Author

XhmikosR commented Oct 26, 2021

I pushed another patch which, in my eyes, is cleaner, but we can go with either way.

The result is still the same:

npm notice === Tarball Details ===
npm notice name:          stylelint
npm notice version:       14.0.0
npm notice filename:      stylelint-14.0.0.tgz
npm notice package size:  149.1 kB
npm notice unpacked size: 780.9 kB
npm notice total files:   356

EDIT: I had to revert it because ESLint wrongfully complained.

Before:
total files:   572
package size:  260.0 kB
unpacked size: 1.4 MB

After:
total files:   356
package size:  149.1 kB
unpacked size: 780.9 kB
@XhmikosR XhmikosR force-pushed the npm-files branch 2 times, most recently from 00d98c8 to cb9444f Compare October 26, 2021 04:48
@jeddy3 jeddy3 merged commit 2ac20d7 into main Oct 26, 2021
@jeddy3 jeddy3 deleted the npm-files branch October 26, 2021 05:09
@jeddy3
Copy link
Member

jeddy3 commented Oct 26, 2021

  • Fixed: package size by excluding docs (#5643).

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

Successfully merging this pull request may close these issues.

None yet

4 participants