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

cds extractor: npm install in each package.json #163

Merged
merged 2 commits into from
Dec 3, 2024
Merged

Conversation

lcartey
Copy link
Contributor

@lcartey lcartey commented Dec 2, 2024

This is because cds dependencies can exist in the package.json that may need to be installed before running the cds compiler.

This is because cds dependencies can exist in the package.json
that may need to be installed before running the cds compiler
@lcartey lcartey requested a review from jeongsoolee09 December 2, 2024 11:44
@@ -36,7 +36,7 @@ then
# directory.
#
# We also ensure we skip node_modules, as we can end up in a recursive loop
find . -type d -name node_modules -prune -false -o -type f \( -iname 'package.json' \) -exec grep -ql '@sap/cds' {} \; -execdir bash -c "grep -q \"^\$(pwd)\(/\|$\)\" \"$response_file\"" \; -execdir bash -c "echo \"Installing @sap/cds-dk into \$(pwd) to enable CDS compilation.\"" \; -execdir npm install --silent @sap/cds-dk \;
find . -type d -name node_modules -prune -false -o -type f \( -iname 'package.json' \) -exec grep -ql '@sap/cds' {} \; -execdir bash -c "grep -q \"^\$(pwd)\(/\|$\)\" \"$response_file\"" \; -execdir bash -c "echo \"Installing @sap/cds-dk into \$(pwd) to enable CDS compilation.\"" \; -execdir npm install --silent @sap/cds-dk \; -execdir npm install --silent \;
Copy link
Contributor

Choose a reason for hiding this comment

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

A quick thought: would it make sense to install @sap/cds-dk globally? Then we don't have to run this find command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately not. We have seen cases of monorepos where different versions of @sap/cds are specified in different directories. The validity of the CDS file format can change between major versions, so it's not always possible to just install one version and have it work everywhere. This approach ensures we install a compatible version of the @sap/cds-dk tools for each directory, minimising the chance of failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then we'll have to do it project by project then. Thanks!

Copy link
Contributor

@jeongsoolee09 jeongsoolee09 left a comment

Choose a reason for hiding this comment

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

Thank you!

@lcartey lcartey enabled auto-merge December 3, 2024 16:40
@lcartey lcartey merged commit ac8dd07 into main Dec 3, 2024
5 checks passed
@lcartey lcartey deleted the lcartey/npm-install branch December 3, 2024 16:59
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.

2 participants