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

Check for invalid input #43

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Check for invalid input #43

wants to merge 16 commits into from

Conversation

fluvf
Copy link
Contributor

@fluvf fluvf commented Jan 24, 2019

Based on #46

  • Add tests for invalid first argument (2fff651)
  • Add test for null and undefined as children (1806e47)
  • Check for invalid arguments (3983985 and 9b45acf)

With this Crel shouldn't crash anymore, if it's accidentally fed garbage

test/index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
@fluvf
Copy link
Contributor Author

fluvf commented Mar 5, 2019

Oh, I haven't look at this in a while... Just a sec, I'll get it rebased and take a look at the requests

@fluvf
Copy link
Contributor Author

fluvf commented Mar 5, 2019

Rebased on #46 so I don't have to do it again anymore. Doesn't touch stuff touched by #47 either, so things should go smooth from here

@fluvf
Copy link
Contributor Author

fluvf commented Mar 6, 2019

Now that I've taken another look at this, after a long break, I'm not too sure about how good of an idea this is. As it doesn't enforce legal tag names, Crel can still crash when createElement is called.
We're introducing some overhead over incomplete checks, that only catch corner cases, that wouldn't / shouldn't make it into production ready stuff anyways. I'm also not sure doing nothing is the best reaction, as it could hide bugs.

So should we do

  • no checks at all (master)
  • incomplete checks (this pr)
  • or enforce correct tag names through regex

And if an invalid tag name is given, should crel:

  • do nothing
  • give an error message
  • or just crash fast so it gets fixed fast

I'd also like to get your thoughts on what to do on empty crel() calls and should / could we introduce some default value to element? Another exposed property, that the user could set maybe?

(For the record, I do think 1806e47 and 9b45acf are good changes tho)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants