Add Joi.func().class() by wswoodruff · Pull Request #1308 · hapijs/joi

wswoodruff · 2017-09-19T02:07:43Z

Add Joi.func().class()
Ensure a function is not valid against Joi.func().class()
Ensure a class is valid against Joi.func().class()

wswoodruff · 2017-09-19T02:19:57Z

Replaces #1305

WesTyler · 2017-09-19T12:34:13Z

lib/is-class.js

@@ -0,0 +1,3 @@
+'use strict';
+
+module.exports = (func) => (/^\s*class\s/).test(func.toString());


This seems... too simple. Haha. I expected to need more than "stringify and see if it says 'class'", but I can't justify that expectation. So.. good job! 👍

Haha, credit to @devinivy's haute lib for the one-liner! https://github.com/devinivy/haute/blob/master/lib/index.js#L157

And in turn to Felix Kling on stack overflow :P! https://stackoverflow.com/a/30760236

Just keep in mind the test used in haute is only good if you already know you have a function.

That's true! We could validate that with a regular string like 'class hahaha Im just a string ya see!'

We should check to make sure func.toString is a function that we can execute before we try to (or that func is indeed a function). Don't want to throw an error here.

Ideally I think it would be worth making sure it is a function as well. So if this was instead typeof func === 'function' && (/^\s*....); that way we know that there should be a toString() method on it as well.

As this code stands right now the following would pass incorrectly:

(/^\s*class\s/).test(['class '].toString()); // true

Next part would be why is \s* being expected in the RegExp? There aren't any leading whitespace characters from the Function.toString() method from what I've seen in the MDN docs nor from my testing. It seems like we should instead just have /^class\s/ as our RegExp.

The whitespace is technically allowed at the beginning because according to the spec "The use and placement of white space, line terminators, and semicolons within the representation String is implementation-dependent." It's possible that that's an impractical concern, but I don't see the harm. At the same time I suppose we're not checking for semicolons and it would clearly be silly to do so, so I see your point.

I am similarly concerned about creating a runtime error if the helper is misused, but it's also valid to say as a precondition that the input to the helper must be a function.

DavidTPate · 2017-09-19T18:13:21Z

test/types/object.js

+            const testFunc = function () {};
+            const testClass = class MyClass {};
+
+            classSchema.validate({ _class: testFunc }, (err, invalidValue) => {


Can we not use the Helper that we have just like in the other tests?

Also, can we add some additional negative test cases such as undefined, null, ['class '], etc. It looks like there are some cases in the current code where we could throw an error instead of actually responding with a Joi error.

Sure thing, updated!

DavidTPate · 2017-09-19T18:19:50Z

lib/is-class.js

@@ -0,0 +1,3 @@
+'use strict';
+
+module.exports = (func) => (/^\s*class\s/).test(func.toString());


We should check to make sure func.toString is a function that we can execute before we try to (or that func is indeed a function). Don't want to throw an error here.

Ideally I think it would be worth making sure it is a function as well. So if this was instead typeof func === 'function' && (/^\s*....); that way we know that there should be a toString() method on it as well.

As this code stands right now the following would pass incorrectly:

(/^\s*class\s/).test(['class '].toString()); // true

Next part would be why is \s* being expected in the RegExp? There aren't any leading whitespace characters from the Function.toString() method from what I've seen in the MDN docs nor from my testing. It seems like we should instead just have /^class\s/ as our RegExp.

DavidTPate · 2017-09-19T18:21:08Z

lib/language.js

        maxArity: 'must have an arity lesser or equal to {{n}}',
-        ref: 'must be a Joi reference'
+        ref: 'must be a Joi reference',
+        class: 'must be an ES6 class'


Wouldn't we just say "must be a class"? We shouldn't be having an ES6 Class and then later an ES7 Class. (This is a bit of a pedantic comment, but I'd rather make it than not)

wswoodruff · 2017-09-19T18:37:20Z

lib/types/object/index.js

    }

+    class() {
+


Should I add a check right here to see if this._flags.func === true?

The way that this is setup right now the flag this._flags.func could have not been set. The reason why being that a user could do Joi.object().class() or Joi.func().class().

Additionally the flag this._flags.func just ensures that we have set the flag that it should be a function, it doesn't validate that the input coming into isClass is actually a function. I would personally use typeof func in the isClass method.

wswoodruff · 2017-09-29T16:12:03Z

@DavidTPate This should be ready for another look!

Marsup · 2017-09-30T00:11:50Z

I'm still thinking about the split.

wswoodruff · 2017-09-30T00:15:34Z

Splitting object and func? Might be a cool idea, I'll bet you're trying to weigh how much work that might be. What you think?

wswoodruff · 2017-09-30T00:25:19Z

If you think there should be a change like splitting func / class out of object, lemme know some of your thoughts on approach and I can give it a shot

DavidTPate · 2017-09-30T13:12:56Z

test/types/object.js

+
+            Helper.validate(classSchema, [
+                [{ _class: ['class '] }, false, null, {
+                    message: 'child "_class" fails because ["_class" must be a Function]',


Interesting, so I'm guessing this is the side-effect of this being off Joi.func() where we have the this._flags.func flag set for validation.

It makes sense given the implementation that we are returned the error "_class" must be a Function my initial thought was that we would get "_class" must be a class. But returning the base type first is the same way it works for any of our other types (ie. Joi.string().uuid())

Yup! So in order to cover the additional function type check, typeof func === 'function' I had to use the is-class.js helper directly in the tests

DavidTPate

The changes look good to me, but also want to get @Marsup's thoughts on this as he mentioned he is thinking about a few things.

Marsup · 2017-10-07T12:39:36Z

Separated the func from the object, can you rebase @wswoodruff ?

Marsup · 2017-10-07T12:41:08Z

test/types/object.js


    });

+    describe('func().class()', () => {


It shouldn't be there, there's a file for function tests.

wswoodruff · 2017-10-07T16:53:28Z

@Marsup Test is failing because eslint doesn't like the object ... rest parameters in joi/lib/types/any/index.js

Marsup · 2017-10-07T19:07:17Z

Why did you take branch v12 in the first place? 😛

wswoodruff · 2017-10-07T19:37:59Z

Ohhh I thought you wanted me to rebase off of v12! My bad

lock · 2020-01-09T07:44:10Z

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

lock · 2020-01-09T07:44:10Z

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

WesTyler approved these changes Sep 19, 2017

View reviewed changes

DavidTPate suggested changes Sep 19, 2017

View reviewed changes

wswoodruff commented Sep 19, 2017

View reviewed changes

Marsup modified the milestones: 11.1.0, 11.2.0 Sep 20, 2017

DavidTPate reviewed Sep 30, 2017

View reviewed changes

DavidTPate approved these changes Sep 30, 2017

View reviewed changes

Marsup reviewed Oct 7, 2017

View reviewed changes

test/types/object.js Outdated

});

describe('func().class()', () => {

Copy link

Collaborator

Marsup Oct 7, 2017

Choose a reason for hiding this comment

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

It shouldn't be there, there's a file for function tests.

wswoodruff changed the base branch from master to v12 October 7, 2017 16:44

wswoodruff changed the base branch from v12 to master October 7, 2017 19:54

Add Joi.func().class()

60852a2

Marsup merged commit bdc7d02 into hapijs:master Oct 7, 2017

Marsup added a commit that referenced this pull request Oct 8, 2017

Cleanup for #1308.

e504a5f

lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020

		@@ -0,0 +1,3 @@
		'use strict';

		module.exports = (func) => (/^\s*class\s/).test(func.toString());

Conversation

wswoodruff commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wswoodruff commented Sep 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidTPate Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devinivy Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidTPate Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidTPate Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wswoodruff commented Sep 29, 2017

Uh oh!

Marsup commented Sep 30, 2017

Uh oh!

wswoodruff commented Sep 30, 2017

Uh oh!

wswoodruff commented Sep 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wswoodruff Sep 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidTPate left a comment

Choose a reason for hiding this comment

Uh oh!

Marsup commented Oct 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wswoodruff commented Oct 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Marsup commented Oct 7, 2017

Uh oh!

wswoodruff commented Oct 7, 2017

Uh oh!

lock bot commented Jan 9, 2020

Uh oh!

lock bot commented Jan 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wswoodruff commented Sep 19, 2017 •

edited

Loading

DavidTPate Sep 19, 2017 •

edited

Loading

devinivy Sep 19, 2017 •

edited

Loading

DavidTPate Sep 19, 2017 •

edited

Loading

DavidTPate Sep 19, 2017 •

edited

Loading

wswoodruff Sep 30, 2017 •

edited

Loading

wswoodruff commented Oct 7, 2017 •

edited

Loading