-
Notifications
You must be signed in to change notification settings - Fork 43
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(app-config-writer): wrong prerequisites check in convert preview-config #2922
base: main
Are you sure you want to change the base?
fix(app-config-writer): wrong prerequisites check in convert preview-config #2922
Conversation
🦋 Changeset detectedLatest commit: 67fb390 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The new sonar issue seems to be a false positive because casting to |
…eview-prerequisites' into fix/app-config-writer/convert-preview-prerequisites
…s and use in app-config-writer
@@ -1,4 +1,4 @@ | |||
export { checkCdsUi5PluginEnabled, enableCdsUi5Plugin, satisfiesMinCdsVersion } from './cap-config'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@heimwege, I'm wondering whether we should keep the export checkCdsUi5PluginEnabled
for backward compatibility reasons. We would then import and re-export the function. I know of other modules that use the function, search in tools suite for checkCdsUi5PluginEnabled
. From a semver perspective this would be breaking changes and require a major version increase, but as we are on a 0.x.y
version here we could do such incompatible changes. We just need to be careful and adjust all consumers, not only in this repo.
Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I adjusted all occurrences of checkCdsUi5PluginEnabled
in open-ux-tools but you are right there might be usages in tools-suite as well. let's keep it.
Or we do it in two steps. First have both in then adjust tools-suite and ensure everything works and then have a follow up PR that gets rid of the import/export in cap-config-writer.
getWorkspaceInfo, | ||
getWorkspacePackages, | ||
minCdsVersion, | ||
hasMinCdsVersion, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a separate export for this, info is also included in checkCdsUi5PluginEnabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently it's additionally being used in cap-config-writer
export function ensureMinCdsVersion(packageJson: Package): void { | |
if (!hasMinCdsVersion(packageJson)) { | |
packageJson.dependencies ??= {}; | |
packageJson.dependencies['@sap/cds'] = `^${minCdsVersion}`; | |
} | |
} |
Co-authored-by: Klaus Keller <[email protected]>
…eview-prerequisites' into fix/app-config-writer/convert-preview-prerequisites
@Klaus-Keller ...and yes w.r.t. the failing unit test I might need your help. The cards-editor-middleware unit test fails because https://cloud.nx.app/runs/OEWcAt6JIL/task/%40sap-ux%2Fcards-editor-middleware%3Atest |
…eview-prerequisites' into fix/app-config-writer/convert-preview-prerequisites
|
cds-plugin-ui5
is currently being searched inpackage.json
of the app to be converted but must be searched for in CAP rootpackage.json
.This fix currently leads to an exception in project-access checkFilesInSrvFolder in case the CAP srv folder contains sub folders. (fixed in #2923)
Depends on #2926 being merged ✅