prevent a try catch from hiding a build error, also fix build/emoji.js in Windows #2291
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…s just in case it runs in Windows and line endings happen to be CRLF
trusktr
changed the title
This is a temporary commit until docsifyjs#2288 and docsifyjs#2291 are merged to Docsify develop, so that I can get Lume build working in Windows (which includes Docsify for the docs site)
| @@ -95,13 +96,9 @@ function writeEmojiJS(emojiData) { | |||
|
|
|||
| console.info('Build emoji'); | |||
|
|
|||
| try { | |||
There was a problem hiding this comment.
@trusktr --
getEmojiData() makes an API call to check if new emoji data is available and update Docsify if necessary. A failed API call does not warrant a broken build (it just means a check for new Emoji data won't occur). This is what the try/catch was in place for.
We can replace the try/catch with something else, but the failed API call should be handled gracefully.
There was a problem hiding this comment.
Why should this succeedd if the API call fails? What's the reasoning?
If this exits zero despite it failing, then other scripts in a pipeline will happily continue along and an issue may go unnoticed. It seems better to not have unintentionally incorrect builds.
What if, for example, we are publishing, and the emoji build fails, and the published package is not up to date because we thought it worked?
There was a problem hiding this comment.
How about this as a simple solution:
- Remove
build:emojifrom thebuildscript inpackage.json - Add
build:emojito thepreparescript inpackage.json - Force failed emoji API call to exit zero (as you suggest) in
build/emoji.js
Answers to your questions below...
Why should this succeedd if the API call fails? What's the reasoning?
The original goals were:
- To prevent GitHub API availability issues or changes from breaking all local and automated builds.
- To allow developers to build and test while offline--specifically, to run the
buildscript.
Remember, the API call is not required to complete a build. This is by design. We do not want our build to be reliant on a remote API that we do not control, which is why emoji data is stored in src/core/render/emoji-data.js. The API call simply checks to see if new data is available and, if so, updates the emoji-data.js file which is used to generate distributable files in /lib/. If the API call fails, the build will complete successfully using the data from emoji-data.js, which is the data stored in our repo and obtained from the last successful API call.
It seems better to not have unintentionally incorrect builds.
Builds won't be "incorrect". Builds will contain "unchanged" emoji data because they will use local emoji data from src/core/render/emoji-data.js.
What if, for example, we are publishing, and the emoji build fails, and the published package is not up to date because we thought it worked?
The result would be that the emoji data used in the newly published package would be identical to the data used in the previously published package.
You make a good point though. We should not rely on developers noticing an error message in the console to ensure our emoji updates are working as expected. See proposed solution at the top of this message.
Summary
I removed the try-catch from build/emoji.js because it prevented the output from being helpful when there was an error, and made the script exit with zero instead of non-zero upon error so a build would pass which would make it even less obvious that there was an error.
Also improve the regex in build/emoji.js so that it works with CRLF or LF just in case of... Windows.
Related issue, if any:
What kind of change does this PR introduce?
Build-related changes
Repo settings
For any code change,
Does this PR introduce a breaking change?
No
Tested in the following browsers:
N/A