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: Correct packageJsonPath determination in ord.js #108

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

nirooxx
Copy link

@nirooxx nirooxx commented Jan 8, 2025

This PR fixes the bug #88 in the ord.js where the packageJsonPath was incorrectly determined using cds.root, leading to errors when running the cds compile command in non-root contexts.
The issue occurred because cds.root does not always represent the working directory when cds compile is executed.
The fix updates the logic to use process.cwd() for determining the working directory, ensuring compatibility in various execution contexts.

Copy link
Member

@aramovic79 aramovic79 left a comment

Choose a reason for hiding this comment

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

@nirooxx : I see 4 tests failing and the code coverage has decreased - now it's below 95%.
cc: @zongqichen

Copy link
Member

@aramovic79 aramovic79 left a comment

Choose a reason for hiding this comment

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

@nirooxx : Reported some minor findings.
cc: @zongqichen @RoshniNaveenaS @Fannon

lib/ord.js Outdated Show resolved Hide resolved
lib/ord.js Show resolved Hide resolved
lib/ord.js Outdated Show resolved Hide resolved
lib/ord.js Outdated Show resolved Hide resolved
@nirooxx
Copy link
Author

nirooxx commented Jan 9, 2025

@nirooxx : I see 4 tests failing and the code coverage has decreased - now it's below 95%. cc: @zongqichen

Should be fixed with npm test -- -u

@aramovic79
Copy link
Member

aramovic79 commented Jan 9, 2025

@zongqichen @Fannon : Could you please also look at the changes(pls consider my comments). The updated snapshots seem not correct to me, I think that ordIds, partOfGroups shall stay the same... Thanks.

cc: @nirooxx

lib/ord.js Outdated
@@ -19,8 +19,13 @@ const cds = require("@sap/cds");
const defaults = require("./defaults");
const path = require("path");

const initializeAppConfig = (csn) => {
let packageJsonPath = path.join(cds.root, 'package.json')
const initializeAppConfig = (csn, modelPath = process.cwd()) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we still need to keep cds.root, since we cds needs to know where is the root of package.json and other config files we want to load.

Copy link
Author

@nirooxx nirooxx Jan 10, 2025

Choose a reason for hiding this comment

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

I've updated it to use cds.root as the primary path, with process.cwd() serving as a fallback for scenarios where cds.root is unavailable, e.g. dynamic contexts

Copy link
Member

@aramovic79 aramovic79 left a comment

Choose a reason for hiding this comment

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

@nirooxx :
I have reproduced the issue #88 locally by running the command cds compile ./erp/srv --to ord -o > tmp/erp_ord.json and putting the break-point in node_modules/@cap-js/ord/lib/ord.js, line 17(function initializeAppConfig).
In both cases, with using cds.root and with using cds.root || process.cwd() the obtained package.json corresponds to the one standing on the same path where the ongoing process is running. In that sense, adding process.cwd() as a fallback doesn't help.
Both functions/commands - cds.root and process.cwd() in this case are returning the same value - path on which the command cds compile... is executed.
Maybe @zongqichen you can give more context and details how you imagined to solve this issue?
To my understanding, the current behavior is correct and can stay as it was.
I don't think we should parse the command arguments (in this case it is path to the service/resource for which we want to generate an ord document: ./erp/srv) and use them as a path to the package.json file, from which we then pick-up the application name(line: const packageName = packageJson.name;). I see no need for such over-engineering.

@zongqichen
Copy link
Contributor

Hi @nirooxx, I tested it locally, the issue still exists:

:~/work/oid-cap-reference-app$ npm install "github:cap-js/ord#fix/github-actions-ord"

added 84 packages, and audited 85 packages in 10s

15 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
:~/work/oid-cap-reference-app$ cds compile ./ariba/srv/ --to ord -o .ariba.json
TypeError: Cannot read properties of undefined (reading 'replace')
    at initializeAppConfig (/work/oid-cap-reference-app/node_modules/@cap-js/ord/lib/ord.js:36:38)
    at module.exports (/work/oid-cap-reference-app/node_modules/@cap-js/ord/lib/ord.js:171:23)
    at .nvm/versions/node/v21.7.1/lib/node_modules/@sap/cds-dk/bin/compile/index.js:320:36
    at Generator.next (<anonymous>)
    at _write (/.nvm/versions/node/v21.7.1/lib/node_modules/@sap/cds-dk/lib/util/write.js:65:28)
    at Object.to /.nvm/versions/node/v21.7.1/lib/node_modules/@sap/cds-dk/lib/util/write.js:40:42)
    at /.nvm/versions/node/v21.7.1/lib/node_modules/@sap/cds-dk/lib/util/write.js:53:49
    at async Object.exec (/.nvm/versions/node/v21.7.1/lib/node_modules/@sap/cds-dk/bin/cds.js:94:16)
:~/work/oid-cap-reference-app$

my expectation is to set the cds.root with correct value, in this case is /work/oid-cap-reference-app/ariba/, instead of where I execute it. I agree with @aramovic79, if it is not worthwhile to spend too much effort on this issue, we can close it.

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.

3 participants