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

(parser) Public API methods should validate input types better #2631

Closed
emanuelb opened this issue Jul 7, 2020 · 3 comments
Closed

(parser) Public API methods should validate input types better #2631

emanuelb opened this issue Jul 7, 2020 · 3 comments
Labels
autoclose Flag things to future autoclose. enhancement An enhancement or new feature help welcome Could use help from community parser

Comments

@emanuelb
Copy link

emanuelb commented Jul 7, 2020

Running hljs.highlightAuto(1)

Result:

Uncaught TypeError: value.replace is not a function
    at escapeHTML (/home/user/node_modules/highlight.js/lib/core.js:45:6)
    at justTextHighlightResult (/home/user/node_modules/highlight.js/lib/core.js:1763:14)
    at Object.highlightAuto (/home/user/node_modules/highlight.js/lib/core.js:1787:18)

it happend when I forget to use .toString() in below code on buffer:

var fs = require('fs');
var buffer = fs.readFileSync("file");
hljs.highlightAuto(buffer)

fix:
ensure code param is in correct type (string) otherwise return error (it's not string, type is X)

@joshgoebel joshgoebel added enhancement An enhancement or new feature parser labels Jul 8, 2020
@joshgoebel joshgoebel changed the title Check code param type to avoid TypeError: value.replace is not a function (parser) Public API methods should validate input types better Jul 8, 2020
@joshgoebel
Copy link
Member

joshgoebel commented Jul 8, 2020

We probably could do better here, but if someone was going to tackle this I'd want a PR that dealt with ALL the public API methods, not just the "issue of the day"... ie fix the whole problem (I've renamed the issue accordingly) I'd prefer not to add a ton of validation clutter to the codebase either - we have a lot of API public methods - I'd want to wrap this kind of thing up in some type of nice abstraction:

// just a rough idea
validateTypes("highlight", arguments,
      ["languageName", "code", "ignoreIllegals","continuation"],
      {
        languageName: "string", 
        code: "string", 
        ignoreIllegals: "boolean?", 
        continuation: "object?"
    })

This raises a few questions:

  • Is there a nice JS type library we could pull in to do this for us - vs writing it from scratch?
  • Would we even want to do that (use an external lib) since currently we are "zero requirements" and always have been...
  • It would have to be a tiny library with no/minimal requirements itself even to be considered I think.
  • Could we somehow do this at compile time using our existing TypeScript typing - is there tooling for that kind of thing?

I'm not opposed to this - if it's done nicely/properly - but we've also managed without it for forever, so I'd say it's not the highest priority IMHO. Leaving this open to continue the discussion.

@joshgoebel joshgoebel added the help welcome Could use help from community label Jul 21, 2020
@joshgoebel
Copy link
Member

This work would pair well with #2277

@joshgoebel joshgoebel added this to the 11.0 milestone Sep 5, 2020
@joshgoebel joshgoebel removed this from the 11.0 milestone Mar 2, 2021
@joshgoebel
Copy link
Member

Periodic issue review. This type of feature is really not helpful once code is in production - and would only add bloat while providing zero or almost zero benefit. It would make some sense perhaps in a debug or development build - which isn't something we currently have. I'm not sure it's worth creating another build target (dev) just to add run-time type validation. Setting this to auto-close.

@joshgoebel joshgoebel added the autoclose Flag things to future autoclose. label Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autoclose Flag things to future autoclose. enhancement An enhancement or new feature help welcome Could use help from community parser
Projects
None yet
Development

No branches or pull requests

2 participants