Extract preview-middleware client code into separate module #1261
Conversation
🦋 Changeset detectedLatest commit: 85dd077 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
|
Getting a warning about peer dependencies when doing |
I cannot fix the chaining 'smells' because that is not supported in UI5 :(. I marked them as
|
@tobiasqueck OK, we can fix it later. |
That is not caused by my changes, you also see it if you checkout @devinea any comment here? |
Those dependencies don't have newer versions so the peer dependencies are older than we use. But that seems to work fine for now. |
There was a problem hiding this comment.
New module packages/preview-middleware-client/ structure and build scripts look fine.
Code coverage is decent.
Not so obvious why sandboxInit.js was renamed to init.ts ? I preferred the sandboxInit.xx FWIW.
Few other comments / suggestions.
More important one is LICENSE which should be consistent with the other modules.
The sandbox name made sense when it was a standalone file next to the html file, now it is moved into a namespace clearly identifying it as a preview coding. Additionally, that namespace will get a lot of additional files for CPE and ADP, so that it would start looking odd if all of them get a sandbox prefix. PS: if you insist I can change the namespace from |
|
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
There is also open discussion about usage of private modules: #1231
What I don't like about this approach is to copy code from private modules. However, I had a discussion with @tobiasqueck and here is a proposal:
- we discourage from using private modules and try to avoid them
- for exceptional cases (like the one in this PR) we except from the rule and use private modules, but keep the possibility open to publish them at a later point in time
There was a problem hiding this comment.
@tobiasqueck Thanks for the updates and clarifications.
I'd echo the comments from @Klaus-Keller also.
This PR extracts the
sandboxInit.jsfrompreview-middleware/templatesinto a separate private module allowing to test this file and to write it in typescript.Details
preview-middleware-clientpreview-middleware/templates/sandboxInit.jstopreview-middleware-client/src/flp/init.tstypes/sapui5also into this new module since they will only be required here (and having them centrally caused lots of instability with the ts language server)