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 merge errors #2410

Merged
merged 2 commits into from
Nov 7, 2023
Merged

fix merge errors #2410

merged 2 commits into from
Nov 7, 2023

Conversation

anoymouserver
Copy link
Contributor

Summary

  • js build was run twice
  • engines were defined twice

Checklist

- js build was run twice
- engines were defined twice

Signed-off-by: anoy <[email protected]>
@anoymouserver anoymouserver added the Skip-Changelog No changelog update is required, minor change label Nov 4, 2023
@anoymouserver
Copy link
Contributor Author

anoymouserver commented Nov 4, 2023

Just discovered that the npm dependencies are actually installed 3x.
First here using the Makefile

news/Makefile

Line 87 in 9c100f2

$(npm) ci

and then twice here as part of the prebuild script (ci does the same thing as install, but more "carefully")
"prebuild": "npm install && npm ci",

I would recommend to remove the "prebuild" step but I'm not sure whether this is the preferred way.
Alternatively it could be renamed to "predev" to only run when executing npm run dev but I don't think that an automated step should execute npm install, which also could modify the package(-lock).json.

@Grotax
Copy link
Member

Grotax commented Nov 5, 2023

I don't see the need for npm install at all in these scripts.

It should just be npm ci.

I think I would prefer to take the npm ci comand out of the makefile and leave that in the package.json.

@Grotax
Copy link
Member

Grotax commented Nov 7, 2023

Thanks for checking this @anoymouserver.

I saw druing testing and building the app that npm ran multiple times but was too lazy to check why.

Now like this much faster build time 👍

@Grotax Grotax merged commit 97aac09 into master Nov 7, 2023
17 of 21 checks passed
@delete-merged-branch delete-merged-branch bot deleted the fix-merge-errors branch November 7, 2023 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip-Changelog No changelog update is required, minor change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants