-
Notifications
You must be signed in to change notification settings - Fork 54
fix(preview-middleware): support rta and cpe for CAP node w/o mockserver #3842
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
base: main
Are you sure you want to change the base?
fix(preview-middleware): support rta and cpe for CAP node w/o mockserver #3842
Conversation
🦋 Changeset detectedLatest commit: 788d443 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
…' of github.com:SAP/open-ux-tools into fix/preview-middleware/support-rta-and-adp-for-CAP-node # Conflicts: # packages/preview-middleware-client/src/adp/api-handler.ts # packages/preview-middleware-client/src/flp/WorkspaceConnector.ts
|
|
||
| await this.setApplicationDependencies(); | ||
| const config:TemplateConfig = { ...this.templateConfig }; | ||
| const config = structuredClone(this.templateConfig); |
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.
@nikmace this change (that should actually be a tiny refactoring and improve the code quality) leads to snapshot updates in test FlpSandbox › router › test/flp.html UI5 1.71 with asyncHints.requests:
from
applications: {"app-preview":{"title":"my.id","description":"","additionalInformation":"SAPUI5.Component=myComponent","applicationType":"URL","url":"..","applicationDependencies":{"manifest":{},"name":"descriptorName","url":"http://sap.example","asyncHints":{"requests":[]}}},"testfev2other-preview":{"title":"My Other App","description":"This is a very simple application.","additionalInformation":"SAPUI5.Component=test.fe.v2.other","applicationType":"URL","url":"/yet/another/app"}}
to
applications: {"app-preview":{"title":"my.id","description":"","additionalInformation":"SAPUI5.Component=myComponent","applicationType":"URL","url":"..","applicationDependencies":{"manifest":{},"name":"descriptorName","url":"http://sap.example","asyncHints":{"requests":[{"name":"myRequest","url":"http://sap.example"}]}}},"testfev2other-preview":{"title":"My Other App","description":"This is a very simple application.","additionalInformation":"SAPUI5.Component=test.fe.v2.other","applicationType":"URL","url":"/yet/another/app"}}
Change is that
{"name":"myRequest","url":"http://sap.example"}
is being added to the asyncHints requests (see commit that adjusts the test snap)
My assumption is that this is done because { ...this.templateConfig } in contrast to structuredClone(this.templateConfig) does not deep clone but only create a shallow copy.
This can lead to unintended side effects because nested objects, like ui5 or apps, are not cloned. Subsequent modifications to config.ui5, config.apps, etc. will also alter this.templateConfig.ui5 / this.templateConfig.apps.
Can you please confirm that the snapshot adjustment is fine and the old snapshot was actually wrong because of the shallow copy? Or do we somehow rely on the shallow copy adjusting the this.templateConfig sub-objects?
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.
Honestly, I don't understand the problem you are trying to solve, and I have no idea how the CAP apps work in the Adaptation Editor. Adding requests to 1.71 might break ADP for 1.71.

This is why we remove it here ^
Therefore, I cannot say how this will affect ADP for UI5 1.71. A little unfortunate that our QA is off this week, and there is an infrastructure problem for the UI integration tests in the tools-suite repo, so we cannot run regression testing.
{ ...this.templateConfig }, this looks unnecessary, and probably you are right about cloning.
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.
As said, there is no actual problem to solve, but using the spread operator to clone an object is actually quite error prone because it only creates a shallow copy whereas structuredClone is a build in clone functionality (that also preserves the type).
Using the spread operator leads to the top-level objects to be cloned but nested properties are still shared with the original object.
Sample:
const originalProject = {
id: 1,
name: "My Project",
settings: {
theme: "light",
fontSize: 12
}
};
// CASE 1: create a shallow copy
const clonedProject = { ...originalProject };
clonedProject.settings.theme = "dark";
console.log("Original Theme:", originalProject.settings.theme); // dark ⚠️ <-- this might not be intended
console.log("Cloned Theme:", clonedProject.settings.theme); // dark
// CASE 2: create a clone
const realClonedProject = structuredClone(originalProject);
realClonedProject.settings.fontSize= "42";
console.log("Original Theme:", originalProject.settings.fontSize); // 12
console.log("Cloned Theme:", realClonedProject.settings.fontSize); // 42And currently there seems to be such a side effect because changing from spread operator to structuredClone actually results in a changed snapshot.
My question is which of the two options is currently implemented:
- The snapshot in
mainis wrong and needs to be adjusted after creating a real clone - We rely on the indirect changes done to the
this.templateConfigvia the shallow copied objectconfig.
In case of 2) this is not documented and is highly confusing and error prone. In that case we should do the changes on the this.templateConfig before creating a clone.
…ta-and-adp-for-CAP-node' into fix/preview-middleware/support-rta-and-adp-for-CAP-node
|
@devinea could you please verify my CodeQL dismissal for |
Agreed. 👍 |
|



Currently API endpoints
/preview/api/*as well as theWorkspaceConnectorignore the namespace that is added by thecds-plugin-ui5resulting in http404responses