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.

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

build: fix TypeError in prism-eslint-hooks.js #18209

Merged
merged 2 commits into from Mar 18, 2024
Merged

Conversation

fasttime
Copy link
Member

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[X] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

This PR fixes an issue in the docs build process that is causing some incorrect rule examples in the indent and indent-legacy rules to be displayed without red markers for errors, for example here.

image

This is due to a TypeError that occurs because of an incorrect condition in another part of the code. This incorrect condition allows the creation of an array with an undefined element where only strings or plain objects are expected as elements:

js TypeError: Cannot read properties of undefined (reading 'content')
    at getTokenContent (/Users/noone/GitHub/eslint/eslint/docs/tools/prism-eslint-hook.js:189:30)
    at splitTokensByLineFeed (/Users/noone/GitHub/eslint/eslint/docs/tools/prism-eslint-hook.js:309:33)
    at splitTokensByLineFeed.next (<anonymous>)
    at splitTokensByLineFeed (/Users/noone/GitHub/eslint/eslint/docs/tools/prism-eslint-hook.js:321:41)
    at splitTokensByLineFeed.next (<anonymous>)
    at splitTokensByLineFeed (/Users/noone/GitHub/eslint/eslint/docs/tools/prism-eslint-hook.js:321:41)
    at splitTokensByLineFeed.next (<anonymous>)
    at convertMarked (/Users/noone/GitHub/eslint/eslint/docs/tools/prism-eslint-hook.js:392:24)
    at convertMarked.next (<anonymous>)
    at /Users/noone/GitHub/eslint/eslint/docs/tools/prism-eslint-hook.js:402:30

The error is visibile in the Netlify logs under Deploy log > Building > Lines 219-306 (last production deploy: https://app.netlify.com/sites/docs-eslint/deploys/65f345a38ee0750008f1a05c#L219-L306) or when manually running npm run build in the docs folder.

Is there anything you'd like reviewers to focus on?

There are no unit tests for prism-eslint-hooks.js. Shall we add one?

@eslint-github-bot eslint-github-bot bot added the build This change relates to ESLint's build process label Mar 15, 2024
@github-actions github-actions bot added the documentation Relates to ESLint's documentation label Mar 15, 2024
Copy link

netlify bot commented Mar 15, 2024

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 3bd27da
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/65f7793b70fbf2000832f988
😎 Deploy Preview https://deploy-preview-18209--docs-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@fasttime fasttime marked this pull request as ready for review March 15, 2024 21:18
@fasttime fasttime requested a review from a team as a code owner March 15, 2024 21:18
@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Mar 16, 2024
@@ -317,7 +317,7 @@ function installPrismESLintMarkerHook() {
continue;
}

if (Array.isArray(token.content) || typeof token.content !== "string") {
if (Array.isArray(token.content)) {
Copy link
Member

Choose a reason for hiding this comment

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

According to Prism typings, token.content can be a string, Token, or array of strings/Tokens, so it looks like this was targeting the Token case, but missed the fact that token itself can be a string, and that's causing the crash?

Would it make sense to keep this condition and fix the problem this way:

for (const token of tokens) {

+    if (typeof token === "string") {
+        yield token;
+        continue;
+   }

    const content = getTokenContent(token);

Copy link
Member Author

Choose a reason for hiding this comment

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

The condition is flawed because Array.isArray(token.content) implies typeof token.content !== "string", but I think it's a good idea to check if token is a string early in the loop nonetheless, because it makes the logic easier to follow. In the below condition it's enough to check that token.content is not a string to verify that it's an array. I wasn't able to reproduce the case where token.content is a Token, but in theory, it should work as well.

Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for fixing the problem!

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit b91f9dc into main Mar 18, 2024
19 checks passed
@mdjermanovic mdjermanovic deleted the fix-prism-eslint-hooks branch March 18, 2024 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion build This change relates to ESLint's build process documentation Relates to ESLint's documentation
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

None yet

3 participants