[FIX] Add 'sap.ui.fl' dependency to manifest.json #318
Conversation
If changes are present during runtime, the 'sap.ui.fl' dependency should be added to the manifest.json so the changes contained in the changes bundled can be applied to the application
|
|
| @@ -20,6 +24,42 @@ module.exports = function({workspace, options}) { | |||
| pathPrefix = `/resources/${options.namespace}`; | |||
| } | |||
|
|
|||
| function updateJson(data) { | |||
| const mLibs = data["sap.ui5"].dependencies.libs; | |||
There was a problem hiding this comment.
can't it happen that there's no dependencies/libs section yet in the manifest?
There was a problem hiding this comment.
at least if createManifest > createSapUi5 > create > depdenencies is running / the creator is doing everything right, it should be fine.
In general the dependencies is flagged as mandatory while I only found a "have to be filled ... for ui5 apps" for the libs section.
There was a problem hiding this comment.
safety first... now within the mandatory part we always have the section not explicitly mentioned as mandatory
There was a problem hiding this comment.
As most mandatory sections are not checked at runtime, I would expect the tooling to do the same. It should not expect that the given manifest is valid w.r.t. the schema.
There was a problem hiding this comment.
I guess with more time, I would have asked for some more changes, e.g.
- move tests related to
generateFlexChangesBundle.jsinto a dedicated module - try to preserve the indent of the original
manifest.json - add another test case for Lonwyr's last commit
But we can improve this in a later release.
Please remember to squash the commits into a single one before merging.
| @@ -20,6 +24,42 @@ module.exports = function({workspace, options}) { | |||
| pathPrefix = `/resources/${options.namespace}`; | |||
| } | |||
|
|
|||
| function updateJson(data) { | |||
| const mLibs = data["sap.ui5"].dependencies.libs; | |||
There was a problem hiding this comment.
As most mandatory sections are not checked at runtime, I would expect the tooling to do the same. It should not expect that the given manifest is valid w.r.t. the schema.
| log.verbose("Writing flexibility changes bundle"); | ||
| await updateFLdependency(); |
There was a problem hiding this comment.
Processors return an array of resources, which should all be written to the workspace, although (currently) in this case it always returns one resource.
But updating the manifest should only happen once, right? So I would expect it to be done after the Promise.all.
| "title": "{{title}}" | ||
| } | ||
| } | ||
| {"_version":"1.1.0","sap.app":{"_version":"1.1.0","id":"application.i","type":"application","applicationVersion":{"version":"1.2.2"},"embeds":["embedded"],"title":"{{title}}"},"sap.ui5":{"dependencies":{"libs":{"sap.ui.layout":{},"sap.ui.core":{},"sap.m":{},"sap.ui.fl":{}}}}} |
There was a problem hiding this comment.
I don't think that this is expected. When modifying the manifest IMO, it should still be formatted, preferably with the same indentation as before, but I think tabs should be fine.
|
See #319 |
If changes are present during runtime, the 'sap.ui.fl' dependency should be added to the manifest.json so the changes contained in the changes bundled can be applied to the application