Fixes for projects that use cds-indexer#378
Open
mihai-herda-SAP wants to merge 4 commits into
Open
Conversation
cds-indexer support, as introduced, called projectInstallDependencies
AFTER running the indexer and used 'npm install --ignore-scripts' for
that install. For projects that depend on cds plugins like @sap/cds-shim,
@sap/cds-mtxs, @cap-js/hana, file-based local plugins, or anything whose
cds-plugin.js entry is wired by a postinstall/prepare hook, this produced
an under-populated model walk:
- The indexer ran first against an empty project node_modules, so plugins
never registered their hooks. The generated index.cds files therefore
failed to transitively include everything that annotation files in the
project reference.
- The full install that followed ran with --ignore-scripts, leaving any
plugin that depends on its own lifecycle hooks (e.g. the project's own
husky 'prepare', or packages whose cds-plugin.js gets emitted by
postinstall) only half-wired even after install completed.
Subsequent 'cds compile db srv app --to json' then failed with dozens of
'Artifact has not been found' errors against service-level projections
and _texts companion entities the plugins were supposed to synthesize.
Two changes:
1. indexer.ts: install full project dependencies BEFORE running the
indexer, so the indexer walks a fully-populated model. Record the
install on project.retryStatus.fullDependenciesInstalled so the
compilation retry path (needsFullDependencyInstallation) doesn't
repeat the same install. If install fails, log a warning and still
attempt the indexer — the cache binary may still produce useful
index files.
2. projectInstaller.ts: prefer 'npm ci' when a package-lock.json exists,
matching what the project locked exactly. Drop --ignore-scripts so
cds-plugin lifecycle hooks can run. Falls back to 'npm install' when
there is no lockfile.
Verified on the SAP ctsm project (~843 cds files, ~1700 deps, mix of
@sap/cds-shim, @sap/cds-mtxs, @cap-js/hana, @cap-js/ord, and a
file-based local plugin): all 'Artifact not found' errors disappear,
leaving only the project's pre-existing modeling warnings — same
output as the working manual flow ('npm ci' + 'npx @sap/cds-indexer'
+ 'cds compile').
Note: this enables npm lifecycle script execution for projects under
analysis. Threat model is comparable to running 'cds compile' itself,
which already loads and executes plugin code from node_modules; some
existing CI tests on Linux may need adjustment for the new install
behavior.
Adjusts the two unit tests that were encoding the old behavior: - projectInstaller.test.ts: the install args no longer include '--ignore-scripts'. Adds a new test for the 'npm ci' branch taken when a package-lock.json exists. Mocks fs/existsSync so the lockfile-detection check is deterministic. - indexer.test.ts: the install now happens BEFORE the indexer, so an indexer failure no longer gates the install. The 'should not install ... when cds-indexer fails' test is replaced with 'should still install ... even when cds-indexer subsequently fails' to assert the new contract. Renames the post-install-failure test to reflect that the install runs first.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the CDS extractor’s handling of projects that depend on @sap/cds-indexer, aiming to ensure the indexer runs against a fully-installed project environment and that installs use lockfile-reproducible semantics when available.
Changes:
- Install full project dependencies before running
@sap/cds-indexerand record the install status in retry bookkeeping. - Prefer
npm ciwhenpackage-lock.jsonexists; otherwise usenpm install, and stop passing--ignore-scripts. - Update Jest tests to reflect the new install-before-indexer behavior and to cover the new
npm cipath.
Show a summary per file
| File | Description |
|---|---|
| extractors/cds/tools/test/src/packageManager/projectInstaller.test.ts | Adds fs mocking and a new test ensuring npm ci is used when a lockfile exists. |
| extractors/cds/tools/test/src/cds/indexer.test.ts | Updates orchestrator tests to expect dependency installation to occur before indexer execution (and even if indexer fails). |
| extractors/cds/tools/src/packageManager/projectInstaller.ts | Adds lockfile detection to choose npm ci vs npm install, and removes --ignore-scripts. |
| extractors/cds/tools/src/cds/indexer.ts | Moves full dependency installation ahead of cds-indexer runs and updates retry status tracking accordingly. |
| extractors/cds/tools/dist/cds-extractor.bundle.js | Updates the committed bundle output to match the source changes. |
Copilot's findings
- Files reviewed: 4/6 changed files
- Comments generated: 2
Comment on lines
514
to
517
| orchestrateCdsIndexer(graph, '/source', new Map()); | ||
|
|
||
| expect(projectInstaller.projectInstallDependencies).toHaveBeenCalledWith(project, '/source'); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two chages were needed in order to make this work on projects that have cds dependencies and also use cds-indexer: