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

RFC: Expand list of ignored files #114

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

Conversation

ruyadorno
Copy link
Contributor

@ruyadorno ruyadorno commented Mar 18, 2020

TLDR; Expand the default list of files that should not end up in published tarball.

See RFC

@ruyadorno ruyadorno added Agenda will be discussed at the Open RFC call Release 7.x labels Mar 18, 2020
@ruyadorno
Copy link
Contributor Author

discussion started here: npm/npm-packlist#48 and the @npm/cli-team decided it would be nice to throw a proper RFC to discuss this a bit more with the community.

@ruyadorno ruyadorno force-pushed the expand-list-of-ignored-files-on-publish branch from 5ee1fe0 to 14189d3 Compare March 18, 2020 22:00
Copy link
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

The real solution imo isn’t to exclude more files, it’s to allow publishers to create two lists: “everything” and “production” - and publish both tarballs, so that by default the smallest tarball installs, but it’s easy to opt in to installing the more complete one.

- `.idea/` (or other editors similar configs/store/etc)
- `.travis.yml`, `.github/` (and/or more ci services)
- `.yo-rc.json` template/boilerplate related files

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.

@ruyadorno
Copy link
Contributor Author

Action items from the OpenRFC call:

  • Restrict this RFC to only items that can be potentially harmful to the ecosystem (high potential of leaking secrets, etc)
  • Let's create a new warning list that would prompt publishers at publish time

@ruyadorno
Copy link
Contributor Author

ruyadorno commented Jul 15, 2020

It would be awesome if folks can contribute to this by suggesting files that they notice have high potential to be harmful if published by mistake to the registry 😊

Copy link
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM pending the one comment

@ljharb
Copy link
Contributor

ljharb commented Jul 15, 2020

What about also ignoring *.sublime-project, bower.json, component.json?

@ruyadorno
Copy link
Contributor Author

from @bnb in OpenRFC call:

We could borrow items from: https://github.com/github/gitignore/blob/master/Node.gitignore

- `.editorconfig` common plugins
- `.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

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!

@darcyclarke darcyclarke removed the Agenda will be discussed at the Open RFC call label Jul 22, 2020
@ruyadorno ruyadorno added semver:major backwards-incompatible breaking changes and removed Release 7.x labels Oct 21, 2020

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

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.


## Detailed Explanation

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

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

@settings settings bot removed the Proposal label Sep 21, 2021
@webstech
Copy link

Is there any incentive to move forward with this? I was shocked to find how many dependabot and workflow files I had in various repos today. Yarn already excludes .github during pack/publish so there is precedent. Internally there seems to be an established list of well known files to blacklist. I found 100+ flavours of dot filenames in local node_modules directories. It may be more code but a warning to suggest using .npmignore or package.files could help with migration. I understand any warnings may not be seen in automation runs - just trying to get some of this to happen.

@ruyadorno ruyadorno removed their assignment Jun 6, 2023
@ljharb
Copy link
Contributor

ljharb commented Jul 14, 2023

Please do not include .editorconfig in this list; I run a tool in my packages that lints against it, and i rely on that file being present.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:major backwards-incompatible breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants