Skip to content

fix(core): do not overwrite package.json start script #15091

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

Merged

Conversation

ondrej-stanek-ozobot
Copy link
Contributor

Do not overwrite app's package.json start script if the app already defines it.

Current Behavior

nx build <app> doesn't fully respect the contents of apps/<app>/package.json. In particular, if the <app>'s package.json specifies custom script such as:

{
  "scripts": {
    "start": "next start -p ${PORT:=8080}"
  }
}

The custom configuration is lost during nx build <app> and the generated dist/apps/<lib>/package.json misses the custom port configuration:

{
  "scripts": {
    "start": "next start"
  },

Expected Behavior

The file dist/apps/<app>/package.json should keep the custom start command, in this example it should be:

{
  "scripts": {
    "start": "next start -p ${PORT:=8080}"
  },

The fix is simple and it is aligned with the existing logic for checking presence of scripts section.

@vercel
Copy link

vercel bot commented Feb 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated
nx-dev ⬜️ Ignored (Inspect) Feb 23, 2023 at 11:53PM (UTC)

@xiongemi
Copy link
Contributor

xiongemi commented Feb 17, 2023

@ondrej-stanek-ozobot the ci did not run. could you rebase and push up again?

@ondrej-stanek-ozobot
Copy link
Contributor Author

@xiongemi , I rebased on latest mainline, but I am unsure if it resolved the problem. Can you please check?

@xiongemi
Copy link
Contributor

xiongemi commented Feb 22, 2023

the pr looks good, but the ci did not get triggered, so checks are not done. the ci needs to be run before the merge. maybe just try to rebase again?

@ondrej-stanek-ozobot
Copy link
Contributor Author

@xiongemi , I rebased again, not sure if it helped. Could you please help to get the CI run completed?

@FrozenPandaz FrozenPandaz merged commit 00c858e into nrwl:master Feb 24, 2023
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants