Skip to content
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.

Sign up for GitHub

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

Jump to bottom

prevent a try catch from hiding a build error, also fix build/emoji.js in Windows #2291

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

prevent a try catch from hiding a build error, also fix build/emoji.js in Windows #2291

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4f62eb8
prevent a try catch from hiding a build error
trusktr Oct 22, 2023
505b685
take into account possible CRLF endings in the regex in build/emoji.j…
trusktr Oct 22, 2023
76334a9
Merge branch 'develop' into unhide-build-errors
Koooooo-7 Oct 22, 2023
a602b60
Merge branch 'develop' into unhide-build-errors
trusktr May 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions build/emoji.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ function writeEmojiPage(emojiData) {
const emojiPage =
(isExistingPage && fs.readFileSync(filePaths.emojiMarkdown, 'utf8')) ||
`<!-- START -->\n\n<!-- END -->`;
const emojiRegEx = /(<!--\s*START.*-->\n)([\s\S]*)(\n<!--\s*END.*-->)/;
const emojiRegEx = /(<!--\s*START.*-->\r?\n)([\s\S]*)(\r?\n<!--\s*END.*-->)/;
// ^ Note, we use \r? in case Windows converts to CRLF
const emojiMatch = emojiPage.match(emojiRegEx);
const emojiMarkdownStart = emojiMatch[1].trim();
const emojiMarkdown = emojiMatch[2].trim();
Expand Down Expand Up @@ -95,13 +96,9 @@ function writeEmojiJS(emojiData) {

console.info('Build emoji');

try {
Copy link
Member

@jhildenbiddle jhildenbiddle Oct 22, 2023 •
edited
Loading

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.

Copy link
Member Author

@trusktr trusktr Nov 23, 2023 •
edited
Loading

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?

Copy link
Member

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:

  1. Remove build:emoji from the build script in package.json
  2. Add build:emoji to the prepare script in package.json
  3. 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:

  1. To prevent GitHub API availability issues or changes from breaking all local and automated builds.
  2. 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.

Copy link
Member Author

@trusktr trusktr May 19, 2024 •
edited
Loading

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).

Copy link
Member Author

@trusktr trusktr May 19, 2024 •
edited
Loading

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).

Copy link
Member

@jhildenbiddle jhildenbiddle May 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Koooooo-7 --

Let's just do this part (and skip the rest):

  1. 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?

Copy link
Member Author

@trusktr trusktr May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exactly what's happening with the build:emoji script.

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:

https://github.com/docsifyjs/docsify/blob/8ef9a3cd036bc86278709199e1da19da36f04ed1/src/core/render/emojify.js

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exactly what's happening with the build:emoji script.

(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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const emojiData = await getEmojiData();
const emojiData = await getEmojiData();

if (emojiData) {
writeEmojiPage(emojiData);
writeEmojiJS(emojiData);
}
} catch (err) {
console.warn(`- Error: ${err.message}`);
if (emojiData) {
writeEmojiPage(emojiData);
writeEmojiJS(emojiData);
}
Loading