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

feat: Add 'changedSince' option #80

Merged
merged 8 commits into from
Nov 15, 2019
Merged

feat: Add 'changedSince' option #80

merged 8 commits into from
Nov 15, 2019

Conversation

ghost
Copy link

@ghost ghost commented Nov 12, 2019

As discussed in #58, this PR adds a new option to wsrun to only run packages which have been changed from a supplied branch. It works exactly like jest --changedSince=master.

It uses jest-changed-files internally.

@ivasilov ivasilov requested a review from spion November 12, 2019 10:38
src/index.ts Outdated Show resolved Hide resolved
src/run-graph.ts Outdated Show resolved Hide resolved
@ivasilov ivasilov changed the title Add changedSince option Add 'changedSince' option Nov 12, 2019
)
}

return filterChangedPackages([...data.changedFiles], this.pkgPaths, this.opts.workspacePath)
Copy link
Contributor

@spion-h4 spion-h4 Nov 13, 2019

Choose a reason for hiding this comment

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

Is the return type Promise<PkgJson[]> ?

Copy link
Member

Choose a reason for hiding this comment

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

No. The expandGlobs function returns Promise<string[]> (in master) so the new function filterChangedPackages has the same signature.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah since it .maps p => p.name first. got it.

README.md Outdated

Misc Options:
--fast-exit, e If at least one script exits with code > 0, abort [boolean]
--collect-logs, l Collect per-package output and print it at the end of each script [boolean]
--no-prefix Don't prefix output [boolean]
Copy link
Collaborator

Choose a reason for hiding this comment

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

no-prefix is not removed, its a way to specify a "false" version for --prefix

Copy link
Member

Choose a reason for hiding this comment

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

I couldn't find reference or usage anywhere in the code. Can you show me where it is used?

Copy link
Contributor

@spion-h4 spion-h4 Nov 13, 2019

Choose a reason for hiding this comment

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

this is a yargs feature when --no-xxx is the same as --xxx=false

Copy link
Member

Choose a reason for hiding this comment

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

I've renamed the flag to be no-prefix because the documentation (when running wsrun --help) would be wrong.

The previous implementation relied on users knowing the --no-prefix yargs trick. The no-prefix line was misleading because it's written as an option but you can't add documentation for it so it's just when running wsrun --help.

Copy link
Author

Choose a reason for hiding this comment

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

i've changed the documentation to include both --prefix and --no-prefix with explanations on their meaning.

@spion spion changed the title Add 'changedSince' option feat: Add 'changedSince' option Nov 13, 2019
@spion-h4 spion-h4 merged commit 2c68739 into master Nov 15, 2019
@spion-h4 spion-h4 deleted the add-changedSince branch November 15, 2019 15:19
@ivasilov
Copy link
Member

🎉 This PR is included in version 5.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@idan-at
Copy link
Contributor

idan-at commented Mar 3, 2020

@spion @ivasilov
Does this also run the tests of the dependent modules?

for example if I have modules A, B, C.
A is dependent on B, and B changed.

Does it only run B or also A ?

@ivasilov
Copy link
Member

ivasilov commented Mar 3, 2020

@idan-at it depends on which additional flags you supply to it.

If you run it as --recursive --changedSince=master on your example (A -> B -> C) and B changed, it will run on B and C.

If you run it as --revDeps --changedSince=master on your example (A -> B -> C) and B changed, it will run on A and B.

You can also combine all three flags.

Does that answers your question?

@idan-at
Copy link
Contributor

idan-at commented Mar 3, 2020

amazing.

Exactly what I needed, thank you :)

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

Successfully merging this pull request may close these issues.

4 participants