SAP / open-ux-tools

Enable community collaboration to jointly promote and facilitate best in class tooling capabilities
Apache License 2.0
83 stars 35 forks source link

BUG - optional flpId is not optional #441

Open tobiasqueck opened 2 years ago

tobiasqueck commented 2 years ago

Related Feature

Feature request: #72

Description

The property is defined as optional at https://github.com/SAP/open-ux-tools/blob/main/packages/fiori-elements-writer/src/types.ts#L85, however, if the property is not provided, then the generated flpSandbox.html file is broken because it is missing the semantic object and intent for the app to be previewed.

Steps to Reproduce

Steps to reproduce the behavior:

  1. Create a simple script with all mandatory input values for @sap-ux/fiori-elements-generator/generate()
  2. Generate an app
  3. Start the (mock) preview

Note: I have only tested this with the Fiori elements writer but I assume the same issue exists in the freestyle writer.

Expected results

The app is loaded

Actual results

The FLP starting page is loaded and there is no tile for my app.

Suggested Solution

I see three possible solutions

  1. Add some code that sets a default value if the property is not provided
  2. Remove the property from the API and always set a default value since there is no reason to actually provide a custom value. In addition, the name flpId is a bit odd since it actually is the serialized intent (semantic object and action).
  3. Remove the property and add a new optional property call navigation of type (https://github.com/SAP/open-ux-tools/blob/main/packages/ui5-config/src/types/manifest.ts#L714). If the object navigation.outbound is provided then we use its content also for the sandbox html, and if not we use a default value
interface navigation {
   semanticObject: string;
   action: string;
   parameters?: { [key: string]: string }
}

My preference is option 3 because it does not just solve the current problem but also prepares the writers for features required when adding deployment configurations.

Version/Components/Environment

Add any other context about the problem here OS:

Root Cause Analysis

Problem

{describe the problem}

Fix

{describe the fix}

Why was it missed

{Some explanation why this issue might have been missed during normal development/testing cycle}

How can we avoid this

{if we don’t want to see this type of issues anymore what we should do to prevent}

devinea commented 2 years ago

@tobiasqueck I suppose minimum required is to add a default value for this parameter.

Splitting flpid into SematicObject and Action makes sense in 3. But we also need to distinguish between writing the flpSandbox.html ( eventually OPA files - I think some reference the intent - historically at least) and the manfiest.json crossNavigation section for deployment. Since the deployment part would be optional. Also, not sure if there is a valid reason for it but these flpSandbox.html and manifest.json intents are often different. So I would favour separating these two intents in the interface at the risk of duplication.

tobiasqueck commented 2 years ago

I think some reference the intent - historically at least)

The only reference to this intent is in the script that we generate into the package.json and that happens within the same writer (https://github.com/SAP/open-ux-tools/blob/main/packages/fiori-elements-writer/src/packageConfig.ts#L16), so there is nobody that I am aware of that references it.

So, if I understand you correctly, you prefer option 2 then (i.e. don't mix this with outbound navigation)