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

Throw error on non-string input #682

Merged
merged 2 commits into from
Jan 20, 2018

Conversation

Feder1co5oave
Copy link
Contributor

@Feder1co5oave Feder1co5oave commented Nov 15, 2015

I agree with what @scottgonzalez said here: #417 (comment)

I chose to return null instead of an empty string in order to let the user know there's a problem, and that it's probably his fault.

Behavior:

> marked()
Error: marked(): input parameter is undefined or null
> var a;
undefined
> marked(a)
Error: marked(): input parameter is undefined or null
> marked(fs.readFileSync('file.txt'));
Error: marked(): input parameter is of type [object Uint8Array], string expected
> marked(0)
Error: marked(): input parameter is of type [object Number], string expected
> marked('')
''
> marked({})
Error: marked(): input parameter is of type [object Object], string expected

The TypeError is still thrown if a user calls manually Lexer.lex and then Parser.parse, but I think it's her/his business at this point.

fixes #168 fixes #417 fixes #447 fixes #291 fixes #442 fixes #635 fixes #658
fixes #681 fixes #755 fixes #758 fixes #776 fixes #787 fixes #794 fixes #802
fixes #934 fixes #867 fixes #903 fixes #955 fixes #1021 fixes #927 fixes #866
fixes #614 fixes #752

@ericclemmons
Copy link

Returning null is correct, IMO. There's no string to parse, so it shouldn't be cast as one.

@BigBlueHat
Copy link

BigBlueHat commented Aug 23, 2016

In my case I was using fs.readFileSync (yeah...I know) and passing it's output to marked() which failed with this replace error. Casting the output through String() did the trick for reasons I know not nor have time (for this project) to dig into...sadly. Hopefully the research is helpful, though!

Having marked() return null while correct would've made debugging things for me worse, though, I agree with @ericclemmons that it's likely the correct behavior technically--or perhaps throwing an deliberate Error would be best.


Well...you know how you post something and then fix your bug?

marked(fs.readFileSync('file.md', 'utf8'))

Does the trick without the String() bit.

@Feder1co5oave
Copy link
Contributor Author

A reasonable alternative could be to return null on null input, and throw an exception on non-string input. That way the programmer can easily see he's calling marked on a wrong type.
It looks pretty pointless to pontificate further though, as it seems marked's development is completely stalled.

@Feder1co5oave
Copy link
Contributor Author

I updated the pull, adding more information for the developer.

Object.prototype.toString is standard.

> marked()
Error: marked(): input parameter is undefined or null
> var a;
undefined
> marked(a)
Error: marked(): input parameter is undefined or null
> marked(fs.readFileSync('file.txt'));
Error: marked(): input parameter is of type [object Uint8Array], string expected
> marked(0)
Error: marked(): input parameter is of type [object Number], string expected
> marked('')
''
> marked({})
Error: marked(): input parameter is of type [object Object], string expected

The code tries to find the object type passed to marked as best it can. This works with builtin objects only, not with user defined classes. (I could work out some code for this too, using src.constructor.name, but it is not supported in IE)

About the "missing tests" label, there's no quick&easy way to test for this, since the test system put in place works by submitting textual input to marked and comparing the output with the expected one. This fix doesn't cover that type of issue.

@joshbruce
Copy link
Member

@Feder1co5oave: Is there a way to test the throw-catch using our current testing setup?

This looks great to me - especially with the updated messages - just want to make sure we've got our bases covered on the automated QA.

@Feder1co5oave
Copy link
Contributor Author

Feder1co5oave commented Jan 5, 2018 via email

@joshbruce
Copy link
Member

joshbruce commented Jan 5, 2018

@Feder1co5oave: Would you mind? Let's see how "dirty" it ends up being. I don't think it's a deal-breaker and if it's gonna be too difficult to put in and throw away - no worries.

@Feder1co5oave
Copy link
Contributor Author

Feder1co5oave commented Jan 5, 2018 via email

@Feder1co5oave Feder1co5oave changed the title Return null on non-string input Throw error on non-string input Jan 9, 2018
@joshbruce
Copy link
Member

Going to merge momentarily. Small change - fixes a lot of things. Just going to confirm a couple of things.

@joshbruce joshbruce modified the milestones: 0.6.0 - Improve developer experience, 0.4.0 - No known defects Jan 20, 2018
@joshbruce joshbruce merged commit 024e378 into markedjs:master Jan 20, 2018
@joshbruce
Copy link
Member

Still seeing two failing tests. What did I miss?

@Feder1co5oave
Copy link
Contributor Author

One (literal_quotes_in_titles) I explained in #1018 (comment)

The other one (cm_link_defs) was born as a conflict between #1018 and #456.
There shouldn't be wrapping dashes in the id of <h1> at line 100 in test/new/cm_link_defs.html:

diff --git a/test/new/cm_link_defs.html b/test/new/cm_link_defs.html
index dec397a..9245002 100644
--- a/test/new/cm_link_defs.html
+++ b/test/new/cm_link_defs.html
@@ -97,7 +97,7 @@ should render to
 
 <h3 id="example-179">Example 179</h3>
 
-<h1 id="-foo179-"><a href="/url">Foo179</a></h1>
+<h1 id="foo179"><a href="/url">Foo179</a></h1>
 <blockquote>
 <p>bar</p>
 </blockquote>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment