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

Use npx command #23686

Closed
wants to merge 1 commit into from
Closed

Conversation

stevenjoezhang
Copy link
Contributor

Proposed change

The build scripts in this repository heavily use ./node_modules/.bin/gulp to execute gulp commands, whereas using npx gulp can achieve the same effect and I believe it's more elegant.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@github-actions github-actions bot added the GitHub Actions Pull requests that update GitHub Actions code label Jan 11, 2025
@bramkragten
Copy link
Member

We should make sure that npx will not go and download gulp but only uses the local version

@wendevlin
Copy link
Contributor

We should make sure that npx will not go and download gulp but only uses the local version

When the package is installed locally npx is using the local one, as far as I know.

@bramkragten
Copy link
Member

We should make sure that npx will not go and download gulp but only uses the local version

When the package is installed locally npx is using the local one, as far as I know.

I know, but if it isn't it will download the newest version online. We should prevent that.

@wendevlin
Copy link
Contributor

I know, but if it isn't it will download the newest version online. We should prevent that.

I don't think npx has a flag for that. We should then only use it in the workflows.
For the build scripts it would make more sense to migrate them to npm scripts and run it with yarn then it will only take the local one.

@steverep
Copy link
Member

First, we shouldn't use npx for anything since we use yarn. The roughly equivalent command is yarn dlx.

However, the right approach here is just to let yarn resolve gulp or any other executables:

yarn run gulp ...

This just uses what we already install. This needs to be done anyway before we could switch to PNP.

@wendevlin
Copy link
Contributor

Thanks @steverep for input. Your way is the right one. I'll close this PR since we want to stay with yarn and want to go the PNP route.

@wendevlin wendevlin closed this Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed GitHub Actions Pull requests that update GitHub Actions code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants