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.
There was a problem hiding this comment.
3. Force failed emoji API call to exit zero (as you suggest) in
build/emoji.js
It was exiting zero, I made it exit non-zero. I don't believe any failure should be silenced.
Moving it to publish script (and publish script failing if the API fails) might be better.
We do not want our build to be reliant on a remote API that we do not control
We're doing that with npm install, relying on some remote server. Same difference. If the build isn't working, I want to know. Up until now (thanks to Windows), I never knew I was possibly missing failed emoji-data build.
The API failure is intermittent, so it is also easy to click the button to re-run failed jobs. The main difference between that and publish script is someone will encounter the issue at a different point less often with a publish script (PRs with builds happening more often than when we publish), but we still want to know of issues (just like we want to know if npm install failed).
There was a problem hiding this comment.
Remember, the API call is not required to complete a build.
Why is that? Will someone try to use an emoji from GitHub that they thought would also work in Docsify but it is missing? If so, I'd say we don't want that, under the premise that the Docsify feature fully works.
If we really want to silence the failure of the API (and sometimes fail to support some emojis?) let's catch that specific failure but not silence the whole script (we want other non-API failures to still fail).
There was a problem hiding this comment.
@Koooooo-7 --
Let's just do this part (and skip the rest):
- A rotate emojiData workflow, running every weekend (or one day?) it looks like the dependabot, if find the changes, raise PR to develop.
- A daily workflow means we don't have to tie emoji checks to a version bump, publishing a new release, merging branches, etc. This is much cleaner approach and avoids having one action (version, publish, merge) fail due to a separate, unrelated action (emoji updates).
- If changes are detected, a PR will be created which will kick off a test run to verify that all emoji tests pass. This will also give us an opportunity to review changes before they are merged.
It's worth mentioning that this approach means it's possible that a new release could be published without the latest updates, but it's highly unlikely. The only two scenarios that would cause this to happen are if we publish a new release when 1) new emoji data is available but our daily emoji update workflow has not run or 2) we ignore an existing emoji update PR.
That works for me. @trusktr? @sy-records?
There was a problem hiding this comment.
This is exactly what's happening with the
build:emojiscript.
The main reason that raised this issue is that the script failed for a reason unrelated to the emoji API, because all errors in the file were supressed. Just want to make this part obvious because it wasn't covered in your reply.
(Side note, the API sometimes fails even if we have internet, due to an API error from GitHub)
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.
I understand that the emoji-data will be unchanged if the script fails. But if GitHub introduces a new emoji :whatever:, and if a user tries to use that new :whatever: emoji but Docsify doesn't render the emoji because the URL is not in that data file, and we release a Docsify update, then the feature could be considered to be partially not working as expected (that's ok, but not the best).
Here in this file:
We can see that it will only process emojis that exist in the emoji-data list. We can also see that Docsify is relying on GitHub as source of truth for names for native emojis based on unicode characters that the API sends back.
(On a related note, if GitHub removes an emoji (unlikely I imagine?) we might also want to show a warning if someone is still using it.)
Based on this, and on
perhaps we should
- move emoji stuff back into the deprecated emoji plugin lol.
- run the "build" on each publish to ensure that GitHub (and named native) emojis align with the release
- without blocking script failure
The only people running the release is one or two of us, so I think it is totally fine if it fails and we try again. We can automate it later if really needed but I don't think it is necessary.
There was a problem hiding this comment.
This is exactly what's happening with the
build:emojiscript.
(Side note, the API sometimes fails even if we have internet, due to an API error from GitHub)
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.I understand that the emoji-data will be unchanged if the script fails. But if GitHub introduces a new emoji
:whatever:, and if a user tries to use that new:whatever:emoji but Docsify doesn't render the emoji because the URL is not in that data file, and we release a Docsify update, then the feature could be considered to be partially not working as expected (that's ok, but not the best).
If we choose to maintain the emojis in our side and sync it from github in every release, it always has the gap that we may miss some latest updated emojis until we do the next release. If we need keep the emoji in real time, we need go back to the beginning solution that concat the emoji directly... 😅
move emoji stuff back into the deprecated emoji plugin lol.
run the "build" on each publish to ensure that GitHub (and named native) emojis align with the release
without blocking script failure
The only people running the release is one or two of us, so I think it is totally fine if it fails and we try again. We can automate it later if really needed but I don't think it is necessary.
I think the emoji plugin solution is good.
Base on the emoji plugin , we could support mount the emojiData instead of keeping it inside (bind the release of docsify itself). Then, if we rotate the emojiData in the emoji plugin on time and do the plugin release asap, it should be more likely up-to-date.
But if users use something like emoji-plugin@v5, it goes back to the same issue tho...
There was a problem hiding this comment.
We need to wrap this up. Way too much time has been spent on this seven month old , arguably low-priority ~10 line change that could be spent on far more impactful PRs.
Moving to an emoji plugin as described in #2431 is not an option for v5. This is why I added "This is a post-v5 feature request (see Milestone)" to the top of the issue when I created it earlier today. v5 needs to be v4 compatible (minus legacy browser support) for all of the reasons we have discussed previously. We can explore #2431 and other breaking options after v5 is released.
The solution proposed in #2291 (comment) addresses the issue described in this PR and all of the concerns shared above. It sounds like @Koooooo-7 is willing to work on it (correct?). If there is a better solution, let's hear it. If not, let's proceed with automating emoji updates via a daily workflow so we all can move on to other tasks.
There was a problem hiding this comment.
I think I may raise a PR for the daily upgrade auto check workflow first, and if we all agree to do it so. we can merge it and set it up to work.
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