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

Fix scripts for npm by extracting run command out #519

Merged
merged 1 commit into from
Feb 23, 2022

Conversation

gosar
Copy link
Contributor

@gosar gosar commented Feb 23, 2022

Currently, for npm the concurrently syntax was concurrently 'npm run-script:build:cjs' which is wrong and didn't work. npm:build:cjs (similar to yarn:build:cjs) is good syntax for concurrently.

Extracting run-script out of PackageManager.getCommand and specifying run directly in package.json scripts (prepack). run is equivalent alias to run-script in npm.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@gosar gosar requested a review from a team as a code owner February 23, 2022 01:16
Copy link
Contributor

@trivikr trivikr left a comment

Choose a reason for hiding this comment

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

@gosar gosar requested a review from trivikr February 23, 2022 01:52
@trivikr
Copy link
Contributor

trivikr commented Feb 23, 2022

This is incorrect as npm run-script is not a packageManager
https://github.com/awslabs/smithy-typescript/blob/d02ff5910ec9e23ee4344b6fcb50a66784b36e2e/smithy-typescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/TypeScriptSettings.java#L396-L398

We should create a new method on PackageManager, say getRunCommand which returns empty string ("") for yarn and "run script" for npm.

The prepack script can be changed to:

"prepack": "${packageManager} ${pmRunCommand} clean && ${packageManager} ${pmRunCommand} build"

and PackageJsonGenerator can contain the additional line:

template = template.replace("${pmRunCommand}", settings.getPackageManager().getRunCommand());

@trivikr trivikr changed the title Fix concurrently to work with npm also Explicitly call run in prepack script Feb 23, 2022
@gosar gosar changed the title Explicitly call run in prepack script Fix scripts for npm by extracting run command out Feb 23, 2022
@gosar gosar merged commit f5c1e59 into smithy-lang:main Feb 23, 2022
@gosar gosar deleted the npm-no-colon branch February 23, 2022 20:31
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.

4 participants