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

Change npm/yarn buildCommand concatenation strategy to always include run #352

Open
kleinfreund opened this issue Dec 5, 2024 · 2 comments
Labels
enhancement New feature or request

Comments

@kleinfreund
Copy link

When configuring buildCommand for the npm and yarn methods, the command is to execute is $executable plus space plus $buildCommand (e.g. npm executable plus build buildCommand results in the command npm build).

This works well for yarn but not npm because npm requires package scripts to be executed via the npm run subcommand. However, yarn (and also pnpm for that matter) support the run subcommand meaning that a command generation using $executable plus space plus run plus space plus $buildCommand would also work for all npm package managers.

Important: This is a breaking change as buildCommand values that already include the run subcommand would break on account of the command starting with $executable run run. This is likely in use, too, as for npm, this workaround is necessary.

Note: This change is only sensible if buildCommand is really only supposed to invoke package scripts and not other yarn/npm/pnpm subcommands which would currently be possible. However, I can't imagine a use case where that would be sensible.

@kleinfreund kleinfreund added the enhancement New feature or request label Dec 5, 2024
@stmh
Copy link
Member

stmh commented Dec 6, 2024

if yarn and npm differs on what they need to build the frontend, then I'd stick to that difference. I tried to be smart in the past (injecting options to silence the output, etc) which had unwanted side effects, as we are not in control of the installed yarn/ npm executable, at least when using the host script-execution-context.

Maybe I didn't get the use-case you have in mind?

@kleinfreund
Copy link
Author

From an authoring point of view of a fabfile, it's a little weird to need to add run when configuring a build command specifically for npm. To specify the build command "build" for npm, users need to currently state run build. But run build is only in the very technical sense the build command. I believe people more commonly would call build the build command (i.e. without the run part).

Yarn doesn't need that the run part because it allows running arbitrary scripts defined in package.json with yarn $scriptName but npm does require the use of the run subcommand. But since Yarn does also support a run subcommand (and so does pnpm), it should be safe to change the command concatenation strategy to make the authoring experience simpler for users of npm.

Then, they can specify buildCommand: build where currently they need to specify buildCommand: run build. This would also make migrations from yarn to npm easier as buildCommand wouldn't need to change at all.

All that being said, this would be a minor improvement to the authoring experience at best.

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

No branches or pull requests

2 participants