-
Notifications
You must be signed in to change notification settings - Fork 14
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
[FIX] Application: Fallback to manifest.appdescr_variant if manifest.json is not found #631
Conversation
…json is not found This fixes a regression presumably present since ui5-projevt 3.0.0, which prevented proper fallback to manifest.appdescr_variant in case no manifest.json is found in the project.
@@ -124,15 +124,15 @@ class Application extends ComponentProject { | |||
try { | |||
return await this._getNamespaceFromManifestJson(); | |||
} catch (manifestJsonError) { | |||
if (manifestJsonError.code !== "ENOENT") { | |||
if (manifestJsonError.cause?.code !== "ENOENT") { |
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.
In general, I don't like if the caller needs to know the potential structure of an exception chain. However, as this is inside the same module, it's acceptable.
Alternatively, the catch clause starting in line 224 could check for the ENOENT and re-throw the err
"as is", without wrapping it. Both error messages anyhow have the same content (no added value).
For the caller this then would result in a direct check on the caught manifestJsonError.
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.
Good idea, somehow I missed that 👍
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.
LGTM
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.
Am I missing something, or is there still no test case that verifies the described fallback behaviour?
@matz3 Something like test/lib/specifications/types/Application.js#L488 ? The test might need some improvement as it didn't detect the issue that this PR fixes. [Update] The mentioned test stubbed the |
This is exactly what I did: https://github.com/SAP/ui5-project/pull/631/files#diff-59c51aa221f82a38efbda063367f94782ab12d1b9d63d9e94e3917ca43f89408R589 This assertion would have failed before the fix. |
Got it, thanks. |
Works like a charm, thank you! |
This fixes a regression presumably present since ui5-project v3.0.0,
which prevented proper fallback to manifest.appdescr_variant in case no
manifest.json is found in the project.