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

Suggest: remove default throw behavior #2276

Closed
joshgoebel opened this issue Nov 12, 2019 · 4 comments · Fixed by #2286
Closed

Suggest: remove default throw behavior #2276

joshgoebel opened this issue Nov 12, 2019 · 4 comments · Fixed by #2286
Labels
enhancement An enhancement or new feature parser

Comments

@joshgoebel
Copy link
Member

joshgoebel commented Nov 12, 2019

Suggestion: We silently swallow errors vs throwing, thereby preserving the page layout and properly rendering at least unhighlighted code blocks (class 'hljs', etc) rather than potentially doing NOTHING leaving the page in an even uglier state.

This should already be an edge case, the question is would it be nicer to go ahead and do our best to recover vs just giving up? I think in many types of failure conditions (such as a single broken/incompatible language) this would be preferable to the WHOLE library failing to work.

--- a/src/highlight.js
+++ b/src/highlight.js
@@ -749,7 +749,12 @@ https://highlightjs.org/
           value: escape(value)
         };
       } else {
-        throw e;
+        console.error(...);
+        return {
+          relevance: 0,
+          value: escape(value),
+          errorRaised: e.message
+        }
       }
     }
   }
@joshgoebel joshgoebel added enhancement An enhancement or new feature parser labels Nov 12, 2019
@joshgoebel joshgoebel changed the title Consider changing default throw behavior Consider removing default throw behavior, and return relevance 0 instead Nov 12, 2019
@joshgoebel joshgoebel changed the title Consider removing default throw behavior, and return relevance 0 instead Consider: removing default throw behavior, and return relevance 0 instead Nov 12, 2019
@joshgoebel joshgoebel changed the title Consider: removing default throw behavior, and return relevance 0 instead Consider: remove default throw behavior, and return relevance 0 instead Nov 12, 2019
@joshgoebel
Copy link
Member Author

joshgoebel commented Nov 12, 2019

Related (examples where making this a softer error could have helped):

@joshgoebel joshgoebel changed the title Consider: remove default throw behavior, and return relevance 0 instead Consider: remove default throw behavior Nov 12, 2019
@joshgoebel joshgoebel changed the title Consider: remove default throw behavior Suggest: remove default throw behavior Nov 12, 2019
@egor-rogov
Copy link
Collaborator

I think in many types of failure conditions (such as a single broken/incompatible language) this would be preferable to the WHOLE library failing to work.

It would be preferable for users, but when you are developing a new grammar you'd like to get errors. Maybe we need both behaviours (some API parameter for a strict mode, turned off by default, but on in developer.html)?

@joshgoebel
Copy link
Member Author

Sure. I’m fine with a flag to enable the prior behavior for debugging/development. Some type of auto detect would be even cooler but I’m not sure what we could reliably key that off of. Perhaps localhost or file:// could auto trigger debug mode or is that over thinking it? In addition to manually enabling.

@egor-rogov
Copy link
Collaborator

Perhaps localhost or file:// could auto trigger debug mode

Sounds reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature parser
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants