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

Allow wireit-only scripts #653

Merged
merged 5 commits into from
Jan 30, 2023
Merged

Allow wireit-only scripts #653

merged 5 commits into from
Jan 30, 2023

Conversation

aomarks
Copy link
Member

@aomarks aomarks commented Jan 29, 2023

Allows defining a script in the wireit section even if it doesn't have an entry in scripts. Such scripts can be used as dependencies of other wireit scripts, but can't be run directly.

This is nice for intermediate scripts that are created only for internal organization, and would not really make sense to invoke directly.

Fixes #644

// This form is useful when using package managers like yarn or pnpm which
// do not automatically add all parent directory `node_modules/.bin`
// folders to PATH.
/^(\.\.\/)+node_modules\/\.bin\/wireit$/.test(command) ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you ever need to handle ./node_moduules/...? Like in a non-monorepo with Yarn>

Copy link
Member Author

Choose a reason for hiding this comment

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

I think if it's installed to ./node_modules/.bin then every package manager should work with just wireit, no need to write ./node_modules/.... npm also automatically checks all parent directory node_modules/.bin, but yarn and pnpm don't do that. So this just handles that gap.

// creating a new object, because other configs may be referencing this
// exact object in their dependencies.
const remainingConfig: LocallyValidScriptConfig = {
...placeholder,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need this spread if you just assign back into placeholder?

Copy link
Member Author

Choose a reason for hiding this comment

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

So that the :LocallyVaildScriptConfig type annotation can be there, so that we know we're ending up with an object that fully satisfies that interface.

src/analyzer.ts Outdated
reason: 'script-not-wireit',
script: placeholder,
diagnostic: {
message: `This command should just be "wireit", as this script is configured in the wireit section.`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: break up long string?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@aomarks aomarks enabled auto-merge (squash) January 30, 2023 00:51
@aomarks aomarks merged commit 2a797c9 into main Jan 30, 2023
@aomarks aomarks deleted the dependencies-not-in-scripts branch January 30, 2023 02:08
@bennypowers bennypowers mentioned this pull request Feb 8, 2023
joshkraft pushed a commit to joshkraft/wireit that referenced this pull request Feb 21, 2023
Allows defining a script in the `wireit` section even if it doesn't have an entry in `scripts`. Such scripts can be used as dependencies of other wireit scripts, but can't be run directly.

This is nice for intermediate scripts that are created only for internal organization, and would not really make sense to invoke directly.

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

Successfully merging this pull request may close these issues.

Don't require dependencies to be in the scripts section
2 participants