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
prevent a try catch from hiding a build error, also fix build/emoji.js in Windows #2291
base: develop
Are you sure you want to change the base?
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this as a simple solution:
- Remove
build:emoji
from thebuild
script inpackage.json
- Add
build:emoji
to theprepare
script 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
build
script.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR:
I think we're getting our wires crossed but both intentions are good. I propose the following:
- Update the console messaging to make it clear that an emoji "update" is unavailable instead of an emoji "build" failing. It's a small change, but I can see how the current wording may have contributed to misunderstanding how and why the emoji build works the way it does.
- Call the
build:emoji
script before updating the project version (currently handled inbuild/release.sh
, called using thepub
andpub:next
npm scripts). - Modify
build:emoji
andbuild/release.sh
to prevent a version bump from happening onbuild:emoji
error or when an emoji update has occurred resulting in uncommitted changes to/src/core/render/emoji-data.js
. The remedy is simply to commit these changes separately so we see the update in the changelog, then continue with the version bump as planned.
This moves us away from checking for emoji updates on every build (which should not break the build because people and APIs go offline) to checking only when Docsify is version bumping in preparation of a new release (which is probably okay since the dev initiating the version bump need to be online to push the new commit(s) and version tag).
FWIW...
Details on how and why the build:emoji
script works the way it does are provided below. I'm sharing these details only because there appears to still be a misunderstanding about how and why our emoji data build/update system works the way it does. Feel free to skip if uninterested.
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.
No. This was explained in #2291 (comment):
"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."
In other words, the worst case scenario of the GitHub emoji API call failing is that a check for and possible addition of new emoji data does not happen. Docsify's emoji support will be unchanged. No new functionality is gained, but no existing functionality is lost. You can review the existing emoji data here: /src/core/render/emoji-data.js
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).
The failure isn't silent. The following message is displayed when the request fails:
Build emoji
- Fetching emoji data from https://api.github.com/emojis
- Error: getaddrinfo ENOTFOUND api.github.com
If you think it would help clarify, we can change the wording to make it clear that an emoji check/upate failed rather than an emoji "build" failing.
We're doing that with npm install, relying on some remote server.
Using your npm install
example to demonstrate...
If you attempt to build Docsify without having run npm install
previously, the build will fail because none of the project's dependencies are available locally. After you run npm install
and have the project's dependencies available locally, you only need npm install
to update the dependencies you previously installed. If you or the remote API/server go offline, you can run npm install package@latest
and see a failure message, then continue building successfully. Sure, you didn't get the package update you wanted, but you can use the package version you installed locally earlier until you or the API are back online.
This is exactly what's happening with the build:emoji
script.
Emoji data has already been fetched, transformed, stored in the repo, and installed on your dev machine when you checked out the project. If you or the remote GitHub emoji API/server go offline, you can run npm run build:emoji
and see a failure message, then continue to build successfully. Sure, you didn't get the updated emoji data you were hoping for, but you can use the emoji data you have in the repo until you or the API are back online.
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.
As per the discussion there , how about that I move the build emoji
stuff to workflow? The changes:
- Make the
build:emoji
throw Exceptions when it failed. - Remove the
build:emoji
from thenpm build
, that we don't fetch the emoji in every build, use thedocsify
maintained emojis by default / always. - Force run the
build:emoji
when we do release (merge to main
) and we need fix it when it is actually failed. Because we should ensure it should be the latest version sync of github at the release date. - An
rotate emojiData
workflow, running every weekend (or one day?)it looks like thedependabot
, if find the changes, raise PR todevelop
. i.e.
update(emoji): sync emojiData and find update emojis in [2024-05-20]
After the changes:
- We can make sure the release won't missing the emoji update, it should sync the latest version in release date.
- We don't need the emoji update in every
build
. The largest out of sync date ofdevelop
emoji isone week
(or one day, depends on the schedule), IMO, it is acceptable. - The
build: emoji
allows users to manually update the emojis if they really want some new released emojis within a weekend (or one day). instead, they need fix the exceptions by themselves (timeout, or something).
@trusktr , @jhildenbiddle Thoughts?
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.
@Koooooo-7 --
Why not just do this part daily:
- A rotate emojiData workflow, running every weekend (or one day?) it looks like the dependabot, if find the changes, raise PR to develop.
- The PR will kick off a test run to verify that all emoji tests pass and we'll have an opportunity to review the changes before they are merged.
- A daily run means we don't have to tie emoji checks to a version bump, publishing a new release, merging
develop
tomain
, etc. This, to me, is much cleaner and avoids one action failing (version, publish, merge) as a result of a separate, unrelated action (emoji updates). - 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See reply here: #2291 (comment)
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