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.

Sign up for GitHub

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

Jump to bottom

RFC: Expand list of ignored files #114

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

RFC: Expand list of ignored files #114

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions accepted/0000-expand-list-of-packlist-ignored-files.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# Expand list of ignored files

## Summary

Let's expand the default list of ignored files in [packlist](https://github.com/npm/npm-packlist).

## Motivation

A large population of npm users are concerned about package sizes and with the advent of the file explorer now available on [npmjs.com](https://www.npmjs.com/) we can now see a number of common files that are very intrinsic to the JS community that we could start ignoring from package bundles without too much friction to the larger ecosystem.

## Detailed Explanation

Expand the current list of ignored files to also ignore by default:
Copy link

Choose a reason for hiding this comment

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

Do the items listed here mean e.g. /.editorconfig or **/.editorconfig? The latter could cause breakage in generators / initializers that have template files in subdirectories, for example https://github.com/pkgjs/create/tree/main/templates


- `.editorconfig` common plugins
ruyadorno marked this conversation as resolved.
Show resolved Hide resolved
- `.gitattributes` and/or more git things
- `.idea/` (or other editors similar configs/store/etc)
- `.travis.yml`, `.github/` (and/or more ci services)
Copy link

@rwjblue rwjblue Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW, removing .travis.yml / .github/workflow/*.yml files would be a breaking change for some packages that use the CI configuration in order to provide users with useful messaging about supported platforms (where the supported platforms are defined as platforms that are under test).

An example would be this code in ember-cli, which is used to emit warnings like:

Node ${process.version} is no longer supported by Ember CLI. We recommend that you use the most-recent "Active LTS" version of Node.js. See https://git.io/v7S5n for details.

Or:

Node ${process.version} is not tested against Ember CLI on your platform. We recommend that you use the most-recent "Active LTS" version of Node.js. See https://git.io/v7S5n for details.

Copy link
Contributor

Choose a reason for hiding this comment

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

wow, TIL people do this.

altho in this case, https://github.com/ember-cli/ember-cli/blob/v3.18.0/.npmignore#L18 would ensure ember-cli is not broken by this change.

Copy link

Choose a reason for hiding this comment

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

wow, TIL people do this.

Ya, our goal was to ensure we could use a single source of truth for this guidance. We could remove it I suppose, but it has come in helpful for things like new Node releases that are untested with their release.

altho in this case, ember-cli/[email protected]/.npmignore#L18 would ensure ember-cli is not broken by this change.

Ya, good point. As long as the .npmignore "wins" (including negation) this won't be an issue. Thank you for looking at that!

- `.yo-rc.json` template/boilerplate related files

Copy link

Choose a reason for hiding this comment

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

.env files would be another good addition.

...and whatever more we think makes sense

## Rationale and Alternatives

Avoiding bundling undesirable files is something we already do today, the idea is only to make it more useful by including some other common files in the JavaScript ecosystem. That said, possible alternatives are:

- Status quo, do not alter the current existing [list of ignored files](https://github.com/npm/npm-packlist/blob/master/index.js#L38).
- More alternatives?

## Implementation

Add some more entries to the already existing [list of ignored files in packlist](https://github.com/npm/npm-packlist/blob/master/index.js#L38) and make sure we have tests asserting it behaves the way we intend.
Copy link

Choose a reason for hiding this comment

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

Suggested change
Add some more entries to the already existing [list of ignored files in packlist](https://github.com/npm/npm-packlist/blob/master/index.js#L38) and make sure we have tests asserting it behaves the way we intend.
Add some more entries to the already existing [list of ignored files in packlist](https://github.com/npm/npm-packlist/blob/main/index.js#L38) and make sure we have tests asserting it behaves the way we intend.


## Unresolved Questions and Bikeshedding

:warning: **Make sure we don't break the ecosystem** - We should definetily err on the side of caution here.