-
Notifications
You must be signed in to change notification settings - Fork 50
Fix(preview-middleware): adjust fallback ui5 version #3722
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?
Conversation
🦋 Changeset detectedLatest commit: 433ef04 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 |
| if (!version) { | ||
| Log.error('Could not get UI5 version of application. Using version: 1.130.0 as fallback.'); | ||
| version = '1.130.0'; | ||
| Log.error('Could not get UI5 version of application. Using version: 1.130.9 as fallback.'); |
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.
Seems odd to have a hardcoded version here at all
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.
The fallback is being used in case requesting the version from the UI5 sources does not work. So If you know of any other option than to hard code it... I'm open for suggestions. As discussed in #3557 (comment) using ui5-info does not seem to be one.
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.
ui5-info does provide fallbacks but not patch version fallbacks. Why is a patch version necessary as a fallback?
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.
ui5-info reads the UI5 version from UI5. So it is not a suitable fallback in case UI5 does not return a UI5 version (see #3557 (comment)).
Not sure why we need to discuss about who needs a patch version, but here we go:
…middleware/fallback-ui5-version # Conflicts: # packages/preview-middleware/src/base/flp.ts
|



spin-off from #3557 (comment)
And also no longer print the fetch error that lead to the fallback as error because its confusing. Changed this to debug.