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

[rush] exclude files from rush change diff calculation #2263

Open
1 of 2 tasks
mmkal opened this issue Oct 7, 2020 · 9 comments · May be fixed by #2279
Open
1 of 2 tasks

[rush] exclude files from rush change diff calculation #2263

mmkal opened this issue Oct 7, 2020 · 9 comments · May be fixed by #2279
Labels
rush-publish-requirements This issue describes user requirements that we should consider in the new design for "rush publish"

Comments

@mmkal
Copy link
Contributor

mmkal commented Oct 7, 2020

Is this a feature or a bug?

  • Feature
  • Bug

Please describe the actual behavior.

If a project edits a test file, and no production code, rush change demands that a changefile be generated even though there are no (meaningful) updates required.

What is the expected behavior?

Allow configuring files to ignore from the diff calculation. One option could be .npmignore, or to use the package.json files property. This may need to follow source maps to avoid false negatives for packages that don't include "source" typescript, though. It may be less complex to pass in directly, e.g. rush change ... --ignore '**/*.test.ts' --ignore '**/*.md', or to put something like "ignoreChanges": ["**/*.test.ts", "**/*.md"] in rush.json.

If this is a bug, please provide the tool version, Node.js version, and OS.

  • Tool: rush
  • Tool Version: 5.34.3
@mmkal mmkal changed the title [rush] exclude certain files from rush change diff calculation [rush] exclude files from rush change diff calculation Oct 7, 2020
@octogonz
Copy link
Collaborator

octogonz commented Oct 7, 2020

This is a great idea. My recommendation is to use .npmignore like rush deploy does.

We try to avoid putting settings like "files" in package.json because (1) that file lacks a strict spec and thus cannot be validated and (2) due to the design of the NPM protocol, the package.json file size impacts installation times.

@mmkal
Copy link
Contributor Author

mmkal commented Oct 7, 2020

Fair enough - I agree that .npmignore is usually a better option that files. One issue with using it for this feature though, copied from the gitter thread:

What if I have src in my .npmignore, because I only publish lib? Then almost all changes would be excluded, unless rush can predict how src/foo.test.ts relates to lib/foo.test.js - which isn't trivial without building first.

@octogonz
Copy link
Collaborator

octogonz commented Oct 7, 2020

@mmkal
Copy link
Contributor Author

mmkal commented Oct 7, 2020

Right - looks like for that library, if I edited something in src, (say, src/Constants.ts), it'd be excluded by .npmignore - it generates lib/Constants.js, which isn't in git, so rush change couldn't compare it to a prior version. So unless I'm misunderstanding something, with this proposal there'd be no changefile generated?

@octogonz
Copy link
Collaborator

octogonz commented Oct 8, 2020

One option could be .npmignore, or to use the package.json files property. This may need to follow source maps to avoid false negatives for packages that don't include "source" typescript, though.

Right, hmm... I didn't realize that. Source maps are not going to be reliable enough.

It may be less complex to pass in directly, e.g. rush change ... --ignore '**/*.test.ts' --ignore '**/*.md'

This is not a good user experience. The most important audience for rush change are people who are totally unfamiliar with how the system works.

or to put something like "ignoreChanges": ["**/*.test.ts", "**/*.md"] in rush.json.

Yeah. It feels like it should be designed as an ignore file like .gitignore and .npmignore, since individual projects may have special requirements. (.changeignore?)

Maybe we could make these files riggable?

@octogonz
Copy link
Collaborator

octogonz commented Oct 8, 2020

CC @iclanton

@iclanton
Copy link
Member

I like the idea of .changeignore with the same syntax and behavior as .gitignore and .npmignore.

I think making this riggable will be more confusing that useful.

@mmkal - is this something you'd be interested in implementing?

@mmkal mmkal linked a pull request Oct 13, 2020 that will close this issue
@mmkal
Copy link
Contributor Author

mmkal commented Oct 13, 2020

@iclanton I opened a PR! #2279

@octogonz octogonz added the rush-publish-requirements This issue describes user requirements that we should consider in the new design for "rush publish" label Mar 9, 2021
@dmichon-msft
Copy link
Contributor

I am of the opinion that the default recursive nesting behavior of .gitignore and .npmignore gets really confusing as well (it's also slow, since it requires a bunch of otherwise unnecessary file system operations) on every transaction. I'd prefer to have a policy of there being exactly one file in each project root, though I guess we can evaluate performance.

@iclanton iclanton moved this to General Discussions in Bug Triage Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rush-publish-requirements This issue describes user requirements that we should consider in the new design for "rush publish"
Projects
Status: General Discussions
Development

Successfully merging a pull request may close this issue.

4 participants