Repository navigation
Conversation
15d0eb5 to
2e094a7
Compare
Extend the parseJSON helper in project and builder packages to use try/catch around JSON.parse, distinguishing empty content (ERR_EMPTY_JSON) from malformed content (ERR_INVALID_JSON, thrown as SyntaxError). Update manifest readers in Application and Component to re-throw ERR_INVALID_JSON unwrapped, so the file path is preserved in the message shown to users instead of being hidden behind a generic "Failed to read" wrapper.
2e094a7 to
85ac842
Compare
| return parseJSON(await resource.getString(), `${this._webappPath}${filePath}`); | ||
| }).catch((err) => { | ||
| if (err.code === "ENOENT") { | ||
| if (err.code === "ENOENT" || err.code === "ERR_EMPTY_JSON" || err.code === "ERR_INVALID_JSON") { |
There was a problem hiding this comment.
Thinking about this more (and looking at the tests), isn't it problematic that the error no longer mentions the project name now?
There was a problem hiding this comment.
I have refactored it not to handle these special cases: e9b7e4e
Now the error handler below will throw them along with its message specifying the project name.
There's a bit redundancy here, but I see this pattern is ok from the code I see in
cli/packages/project/lib/specifications/types/Library.js
Lines 418 to 435 in eda5b70
I would appreciate if you have any ideas on how to handle this better and more generic.
| return JSON.parse(await resource.getString()); | ||
| return parseJSON(await resource.getString(), `${this._srcPath}${filePath}`); | ||
| }).catch((err) => { | ||
| // Do not handle parseJSON's exceptions independently since some information will be lost. |
There was a problem hiding this comment.
While this comment is really not wrong, it seems to be documenting code that is not there (anymore).
I find that a bit confusing? Does it really help, without knowing the history of this PR, or should we rather remove it?
| }).catch((err) => { | ||
| // Do not handle parseJSON's exceptions independently since some information will be lost. | ||
| // See the Error below | ||
| if (err.code === "ENOENT") { |
There was a problem hiding this comment.
Now you added this new if clause for Component.js. That's fine (although I thought you wanted to do that rather in a future change?) - but then we should also add it in Library.js
This branch has not been deployed
2 participants
JIRA: CPOUI5FOUNDATION-1046