new 'check' function on devel

[Nick Martin]

Hey all,

Now on devel is a new function `check` you can use to ensure that a value matches a required type and structure. This is useful to validate untrusted input from the client. Some example calls would look like this:

check(name, String);

check(arguments, [String]);

check(options, {foo: String, bar: Number})

You can also pass an arbitrary matching function:

var Coordinate = Match.Where(function (x) {

  check(x, Number);

  return x >= 0 && x <= 1;

});

check(xCoord, Coordinate);

Docs are here: http://0.6.2-match-docs.meteor.com/#match

We also wrote the audit-argument-checks package. This package causes Meteor to require that all arguments passed to methods and publish functions are checked. Any method that does not pass each one of its arguments to check will throw an error, which will be logged on the server and which will appear to the client as a "500 Internal server error". This is a simple way to help ensure that your app has complete check coverage.

I've built a release that includes the new packages. You can run it with meteor --release 0.6.2-match

Feedback welcome!

-- Nick

[Andrew Wilcox]

I do ad-hoc and predicate checks a lot more than I look for specific constructors, thus personally I'd prefer for the function value pattern slot to be given to Match.Where and to have a Match.Is for constructors:

check(x, _.isFinite);

check(x, Match.Optional(EJSON.isBinary));

check(x, function (n) { return n > 0; });

check(x, Match.Is(Date));

(To delve a bit deeper, in my experience the OO idea of trying to force types to be one-to-one with implementation classes ends up being awkward except for simple cases, and so in practice I tend to find predicates easier and more natural as the code gains some real-world complexity).

Here's a another idea. I normally wouldn't suggest something crazy like this, but this is a DSL after all.  Function values could default to a Match.Where, but instead do an instanceof check if the fn.name starts with an uppercase letter.  (If desired the developer can use an explicit Match.Where or Match.Is to force one or the other).

[Ian Serlin]

I like where this is going and can see it used as the basic infrastructure for model validation performed in #allow/#deny definitions. But a decent amount would have to be added to support that, including the ability to return an array of validation failures. Model validation (like input guarding currently) is still one of those things you must have and must write "from scratch" at the moment.

A validate function built on top of check that took an object definition that specified an array of validations to be performed on each key in the object and returned the caught array of errors would get us pretty far. Is that consistent with the longer-term vision of this feature?

- Ian Serlin

PS - audit-argument-checks seems draconian, I like it. :)

-------

Ian Serlin

i...@ianserlin.com

415-754-9563

http://twitter.com/spiceninja
http://www.linkedin.com/in/ianserlin

--
You received this message because you are subscribed to the Google Groups "meteor-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to meteor-core...@googlegroups.com.
To post to this group, send email to meteo...@googlegroups.com.
Visit this group at http://groups.google.com/group/meteor-core?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Nick Martin]

@Andrew:

I think you're right that predicate checks will be more common than constructor checks.

We did constructor functions as it makes the syntax more parallel, eg check(x, String) and check(x, Date). All the other checks are about enforcing something is of a type, Match.Where is the odd one out so it felt natural to call that one out.

One potential weirdness around Match.Is is string objects. That is, without special care `Match.Is(String)` and `String` would do something different (new String(foo) is an Object, not a String). There's an XXX about boxed string objects in the code. I have no idea when people actually use boxed objects in the wild =)

The idea of keying on fn.name is interesting, but I think a bit too magical =). Imagine someone refactoring their code and changing a name and all of a sudden their code breaks.

Riffing on that, though, what about a bare function can return true, false, or an Object. If it is true/false (or null or undefined, I guess), treat it like a predicate. If it returns an Object, treat it like a constructor function. It might be a bit confusing to explain, but I think it does what you mean in most cases. And it could save a lot of typing 'Match.Where'.

Will ponder.

@Ian:

Yeah, check is def a low-level API. We were def thinking about higher level APIs for validation and such when designing match. That's what I think Match.test will be useful for, though how people will actually use it I don't know =). We also talked about something like Match.match which would return a more complex result detailing the match error, but we couldn't come up with a reasonable API in the few minutes we talked about it. I figure it will be easy to add later after having seen some real world usage.

Re audit-argument-checks: The initial sketch had the audit package just enforce that you call 'check' at least once. Less draconian, but too easy poke holes in. The full on checking also turned out to be cleaner in code =). I suspect it will actually feel pretty lightweight in practice, it feels pretty natural to add one check statement per argument and pass the whole argument to check.

Cheers,

-- Nick

[Andrew Wilcox]

Riffing on that, though, what about a bare function can return true, false, or an Object. If it is true/false (or null or undefined, I guess), treat it like a predicate. If it returns an Object, treat it like a constructor function. It might be a bit confusing to explain, but I think it does what you mean in most cases. And it could save a lot of typing 'Match.Where'.

I'm not following you... If I pass a function as a match pattern, you're going to call it and see if it returns an Object?  How does this help with constructor tests?  Can you give an example?

[Nick Martin]

Yeah, you're right. I wasn't thinking it through =). You'd have to call the function with the single argument of what to check (like a predicate), but the constructor may take something else and get very confused.

On Fri, Apr 19, 2013 at 3:54 PM, Andrew Wilcox <andrew...@gmail.com> wrote:

Riffing on that, though, what about a bare function can return true, false, or an Object. If it is true/false (or null or undefined, I guess), treat it like a predicate. If it returns an Object, treat it like a constructor function. It might be a bit confusing to explain, but I think it does what you mean in most cases. And it could save a lot of typing 'Match.Where'.

I'm not following you... If I pass a function as a match pattern, you're going to call it and see if it returns an Object?  How does this help with constructor tests?  Can you give an example?

[Andrew Wilcox]

One potential weirdness around Match.Is is string objects. That is, without special care `Match.Is(String)` and `String` would do something different (new String(foo) is an Object, not a String). There's an XXX about boxed string objects in the code. I have no idea when people actually use boxed objects in the wild =)

Don't think you do want "new String('foo')" to pass the common string test, because primitive strings and boxed strings work differently.  No one does pass boxed strings around -- even the String methods return primitive strings -- so you're just creating an attack vector if you allow a boxed string to pass your usual string test.

primitive string:

> eval("1 + 1")
2

> EJSON.stringify('foo')
""foo""

String object:

> eval(new String("1 + 1"))
String {0: "1", 1: " ", 2: "+", 3: " ", 4: "1"}

> EJSON.stringify(new String('foo'))
"{"0":"f","1":"o","2":"o"}"

You could fix EJSON of course, but that's just an example: if you allow boxed strings then you have to verify that every library you use does the right thing... for no good purpose, really.

So I'd prefer that the common string test be typeof(x)==='string' (thus excluding boxed strings), and then if I really want to allow boxed strings I can make a special exception.

If this argument is convincing then check(x, String) no longer makes sense anyway, if it's not going to match a boxed string that is instanceof String.  At this point you might as well just define some useful global predicates:

check(x, isString);

which is not as pretty, sadly, but I'd treat the boxed classes as an obscure bit of Java-envy that is best avoided.

[fritz....@gmail.com]

In my opinion the API feels a bit clustered. E.g. I don't see the need for Match.Where. The second parameter of `check` is the callback to check for - why not make it a callable always. Every other constraint can be expressed as a callable instead:

Match.any = function (value) { return true; };
Match.Object = function (objectSpec) { return function () { ... }; }

check(value, Match.any);
check(value, new Match.Object({});

This is pretty close to what the API already is like.  I just don't see the need for the extra Match.Where trip.

About naming: having seen the name `check` I wouldn't expect it to throw an error if the check does not pass, I would expect it to return true/false as test does. A better name would be assert.
Namspacing: I don't like the idea of check being in the global namespace :)

Love the audit-package, should be enabled by default ;)

[Andrew Wilcox]

Ah, here's an idea.  Currently lowercase "string", "boolean", "number" is available in the namespace.  So "string" could be a function which checks if its argument is a string, and we could say

check(roomId, string);

This gets away from testing by constructor, which I like, because I don't allow boxed strings in my code... the test I use is typeof(x)==='string'.  If the standard Meteor check function allowed boxed strings, I wouldn't use it, I'd continue to use the typeof test.  I like the lowercase naming because I think of primitive types as being lowercase as returned by typeof.

I prefer this to my first suggestion of "isString" etc. because checking for strings, booleans, etc. is so common, and it reads better.

I also like getting away from constructors because I often want a more restrictive test.  I am almost never testing for just a Number, for example.  I'm looking for an integer, or a positive integer, or a non-negative integer, or at least a finite number -- I rarely have an API where I'm going to do something useful with NaN or -Infinity.

On Monday, April 22, 2013 7:59:22 AM UTC-4, fritz....@gmail.com wrote:

...

Namspacing: I don't like the idea of check being in the global namespace :)

I don't think we need to worry about cluttering the global namespace.  If you look at the linker branch under development you'll see some nice code for imports -- imported variables become available to your code through static scoping without going into the global namespace.  And while I don't know what's going to make it into the first released version, I can imagine that it might not be hard to eventually have easy control over what does get imported into your code -- so if it's your preference you could refer to "check" (or any other normally imported variable) through an explicit reference under your control instead through an implicit import.

[Andrew Wilcox]

I threw together a branch for check using predicate functions:

https://github.com/awwx/meteor/tree/check-using-predicate

Also available as a PR: 

https://github.com/meteor/meteor/pull/996

[Nick Martin]

Hrm... not so sure about adding 'string', 'boolean', 'number' and 'object' to the global namespace. It's true adding 'check' is already a deviation from our normal pattern of capitalized version of the package name, but I think having 'string' and 'String' both exist and mean different things may be a bit over the top / confusing for noobs.

-- Nick

[Andrew Wilcox]

On Tuesday, April 30, 2013 4:23:37 AM UTC-4, Nick Martin wrote:

Hrm... not so sure about adding 'string', 'boolean', 'number' and 'object' to the global namespace. It's true adding 'check' is already a deviation from our normal pattern of capitalized version of the package name, but I think having 'string' and 'String' both exist and mean different things may be a bit over the top / confusing for noobs.

Do you have a preference for what to use?  "isString"?

 

[Andrew Wilcox]

 I think having 'string' and 'String' both exist and mean different things may be a bit over the top / confusing for noobs.

Maybe, though using "String" is worse on the potential confusion front.

> "foo" instanceof String
false

However personally, as long as the check construct for strings has the right interpretation of "a primitive string" and is clearly documented, I don't care if it's named "String" or "string" or "isString".

[Andrew Wilcox]

OK, let me reconsider...

String, Number, Boolean, undefined, null
Matches a primitive of the given type.

These are fine.  The documentation does explicitly say that these match the primitive of the type (which is the right decision), and it doesn't claim that these are constructor tests (which would be both confusing and wrong).

Any constructor function (eg, Date)
Matches any element that is an instance of that type.

We did constructor functions as it makes the syntax more parallel, eg check(x, String) and check(x, Date).

Ah, but constructor tests often aren't very useful.  For example, your own Object doesn't match arrays (even though [] is instanceof Object) and your String matches primitive strings (even though "foo" isn't instanceof String).  This happens all the time :-)  

 

All the other checks are about enforcing something is of a type.

Type, yes.  instanceof, no.  A predicate function is the right choice to use for a type test, if you get away from the OO misidentification of type === class and use "type" in the useful mathematical sense of being able to test whether an object is a member of set or not.  (Which is the kind of check you want for validation anyway).

I think I got confused by "makes the syntax more parallel" and starting thinking of String etc. as constructor tests in parallel with Date being a constructor test, and that's why I introduced lowercase "string" as an alternative.  But the current documentation and implementation of String etc. is fine.

So I think the only change I need to advocate for is being able to use the function pattern slot for predicates.

Andrew

[Andrew Wilcox]

OK!  New PR:

https://github.com/meteor/meteor/pull/1005

This one is much simpler.

It gives the function pattern slot to predicates, while leaving String etc. alone.

Along with making check easier to use, it's also a win in code conciseness (more Match.Where's were able to be removed than Match.Is's had to be added).

[Andrew Wilcox]

Aha!

In my last proposal where I had moved matching constructors to Match.Is, I added a check for some common constructors being passed in by mistake as a bare match pattern.  When I detected that a constructor was being passed in as a bare match pattern, I threw a helpful error.

Then when I looked at the code I had written, and I thought, "why am I identifying these constructors as constructors, so that I know that they're constructors, and then throwing an error instead of letting the user use them as constructors?"

So my latest proposal is that we allow constructors to be registered with the match system so that it knows that they are constructors, and then they can be used for an instanceof test without having to use Match.Is.

And that allows other functions to be used as predicates, without having to wrap them in Match.Where.  Which is a win because predicates are common.

This proposal is up as a PR at https://github.com/meteor/meteor/pull/1008

[Andrew Wilcox]

P.S.  I'm not sure about the API for registering constructors... I used Match.constructors to be in parallel with Meteor.methods, but maybe Match.registerConstructor or something would be better (though I'm not sure I really like "registerConstructor" either...)

[Morten Henriksen]

Just some input, may do what you like:

I think the usage would typically be to verify types in an object / document before allowing it eg. into the database.

I many other ORM systems we define a type + validation object,

Maybe make a Meteor.Match( doc, docType ); // Where the document i simply the object to test and the docType is the object to validate against.

docType = {

  name: { type: 'string', required: true }, // Validation options

  email: { type: 'email', required: true, errorMsg: 'please.enter.email' }, // Maybe future getText notation for languages

  age: { type: 'number', min: 0, max: 120 },

  photo: { type: 'base64', maxLength: 1024*1024}

};

The type referes to the validation function to use - could be custom added - the validation function takes the field and the option object as parametre something like eg.:

Meteor.Match.validators({

  'string': function(fieldValue, options) {

    if (fieldValue === '' + fieldValue || (fieldVaule === undefined && !options.required)) {

      // Test if any maxLength constrains

      if (options.maxLength != undefined)

        return (fieldValue.length <= options.maxLength)

      else

        return true;

    } else

      return false;

  }

});

Point being that one can define data constrains in a common server/client code an easily use it throughout the system.

[Andrew Wilcox]

On Thursday, May 2, 2013 9:27:33 AM UTC-4, Morten Henriksen wrote:

docType = {

  name: { type: 'string', required: true }, // Validation options

  email: { type: 'email', required: true, errorMsg: 'please.enter.email' }, // Maybe future getText notation for languages

  age: { type: 'number', min: 0, max: 120 },

  photo: { type: 'base64', maxLength: 1024*1024}

};

Is there anything preventing you from writing a package yourself that would do this, building on top of the functionality provided by the check system?

[Morten Henriksen]

Ok... Well, I'll just be a bit more concrete:

• I dont like check in global space so one could maybe be constructive and consider Match(foo, bar); instead of a global check
  
• The talk about adding global lowercase 'string' etc. gave me the goose... But after reading the actual code I see that its the builtin types used for the scheme
• One could consider the possible options to a match... eg.: Match(foo, String, { maxLength: 10, required: true } );
  
• I kind of agree with fritz - Throwing an error is a bit overkill - we kindof expect the validation to do two things fail/succes - false/true (otherwise we have to write a try catch every time...)
  
• I did like the: Match.Where ( though its only a readable wrapper, but nice like indents, spacing etc. making the code more readable )
  

var docType = Match.Where(function(data, options) { // Added options for this

   // I could have an option.log if I wanted to give some feedback to a form or something like that

   // if (! Match(data.email, Validators.Email, { required: true }) ) options.log.push('Enter.correct.email');

   // 

   return Match(data.name, String, { required: true }) && 

            

           Match(data.number, Integer, { min: 0, max: 120 }) && 

           Match(data.photo, Validators.base64, { maxLength: 1024*1024 });

});

if (Match(foo, docType)) thenIDontHaveToMakeMyOwnPackage();

[Andrew Wilcox]

On Thursday, May 2, 2013 11:14:34 AM UTC-4, Morten Henriksen wrote:

Ok... Well, I'll just be a bit more concrete:

• I dont like check in global space so one could maybe be constructive and consider Match(foo, bar); instead of a global check
  

Today packages export variables by placing them into the global namespace.  With the linker, they'll be imported into the code you're using if you are personally using a package such as "check".  Eventually we can imagine you might have the option to use a package without default imports, so you don't have to have variables imported into your namespace if you don't want them.

• The talk about adding global lowercase 'string' etc. gave me the goose... But after reading the actual code I see that its the builtin types used for the scheme

I've withdrawn my own proposal for lowercase 'string' etc., so as far as I know no one is advocating for them now.

• I kind of agree with fritz - Throwing an error is a bit overkill - we kindof expect the validation to do two things fail/succes - false/true (otherwise we have to write a try catch every time...)
  

You don't need to use try-catch because Match.test returns true/false.

[Andrew Wilcox]

I've updated my proposed PR so that the function to register a constructor is now called "registerConstructor".  I think it reads a lot better now.

I moved the ability to use Date, RegExp, etc. up in the documentation (before they were buried down in the description of the constructor match pattern).

[Nick Martin]

Hey Andrew,

Geoff and I sat down today to talk about this. I convinced him that passing an anonymous function should work as a predicate. He came up with a good idea for doing this while still allowing object type checks, based on a trick used on the controllers branch.

The idea is to check for the presence of any keys in the function's prototype. If there is at least one key in the prototype, treat it as an object and do an instanceof check. If it has no keys, treat it as a predicate function and call it.

I think this gives the best of both worlds! It means the simple syntax does what you expect, eg:

`check(value, function (x) { return some_calculation(x); })`

`check(value, Animal)`

`check(value, [Animal])`

This is in some sense heuristic, but I think the cases where it doesn't work are very rare, why have an object with no methods? Also, object constructors don't typically return a value, so it will fail safely (always failing the check) if for some reason you do pass an object with no prototype.

Since this is slightly heuristic, it's probably still good to include an explicit operator for forcing an instanceof check. Geoff sold me on the name `Match.InstanceOf` instead of `Match.Is`. It's OK that it's long, since people won't have to type it much, and it is much more descriptive, and easier to remember since it matches the javascript keyword.

We'll still need to special case the basic javascript types. It looks like Date doesn't have any keys on it's prototype, presumably because its native and weird.

What do you think? Can you whip up a PR with this technique?

Cheers,

-- Nick

On Tue, May 7, 2013 at 4:07 AM, Andrew Wilcox <andrew...@gmail.com> wrote:

I've updated my proposed PR so that the function to register a constructor is now called "registerConstructor".  I think it reads a lot better now.

I moved the ability to use Date, RegExp, etc. up in the documentation (before they were buried down in the description of the constructor match pattern).

[Andrew Wilcox]

We'll still need to special case the basic javascript types. It looks like Date doesn't have any keys on it's prototype, presumably because its native and weird.

Actually its prototype keys just aren't enumerable: 

> Object.getOwnPropertyNames(Date.prototype)
["constructor", "toString", "toDateString", "toTimeString", "toLocaleString", ...

>Object.keys(Date.prototype)
[]

Anyone can makes their keys not enumerable if they want to (it's not a native thing), though I don't know why the JavaScript library classes choose to.

This is in some sense heuristic, but I think the cases where it doesn't work are very rare, why have an object with no methods?

You mean like Meteor.Error?  :-)

 

What do you think? Can you whip up a PR with this technique?

You'll need to test / get it to work on on whatever older versions of IE you want to support, I'm not set up to develop against them.

[Nick Martin]

On Wed, May 8, 2013 at 3:50 AM, Andrew Wilcox <andrew...@gmail.com> wrote:

We'll still need to special case the basic javascript types. It looks like Date doesn't have any keys on it's prototype, presumably because its native and weird.

Actually its prototype keys just aren't enumerable: 

> Object.getOwnPropertyNames(Date.prototype)
["constructor", "toString", "toDateString", "toTimeString", "toLocaleString", ...

>Object.keys(Date.prototype)
[]

Anyone can makes their keys not enumerable if they want to (it's not a native thing), though I don't know why the JavaScript library classes choose to.

Ah, cool! Good to know. Looks like getOwnPropertyNames isn't supported in IE8, though. Perhaps there is another way to find the prototype's keys in IE8.

 

This is in some sense heuristic, but I think the cases where it doesn't work are very rare, why have an object with no methods?

You mean like Meteor.Error?  :-)

Hrm, yeah. So there are a few cases where we still need `Match.InstanceOf`

 

What do you think? Can you whip up a PR with this technique?

You'll need to test / get it to work on on whatever older versions of IE you want to support, I'm not set up to develop against them.

We've got scripts to run the unit tests in all browsers, and the match tests are pretty good so unit tests passing is good enough.

Since you've been so involved with this, I figured you'd want to see it through. If you'd rather hand it off at this point, though, I can get it done.

-- Nick

[Andrew Wilcox]

Wow, gmail's new "improved" UI gave me not a clue that I was replying to the list instead of individually :-o  

Ah, cool! Good to know. Looks like getOwnPropertyNames isn't supported in IE8, though. Perhaps there is another way to find the prototype's keys in IE8.

Yes, though as far as I know there isn't a way to make properties non-enumerable in IE8, so I think if we fallback to _.keys() or equivalent if getOwnPropertyNames isn't available we'll be fine.

My only other thought is to walk up the inheritance chain using Object.getPrototypeOf (if available) or obj.constructor.prototype (which can be forged, but not a problem with trusted code on the server).  That way we could support classes that themselves don't have any methods, but are a subclass of a class that does.  (Pretty common for subclasses of Error for example).

We've got scripts to run the unit tests in all browsers, and the match tests are pretty good so unit tests passing is good enough.

Cool!

 

Since you've been so involved with this, I figured you'd want to see it through. If you'd rather hand it off at this point, though, I can get it done.

Thanks!  My first round of writing code was useful because it helped shake some ideas loose which I otherwise wouldn't have thought of... but I'm deep in the middle of offline data and would be happy to let you take it :)  

[David Greenspan]

DontEnum was a native-only thing until recently (ECMAScript 5, i.e. Firefox 4 and IE 9).  Built-in prototypes use it so that you don't see methods in your for-var-in loops -- if Object.toString was enumerable, you'd see it when iterating over any object!

It was a short-sighted design decision of JS to make methods on prototypes show up in these iterations ever, but now we have to cope with it using some combination of: not adding methods to prototypes (not putting user-defined methods on Object.prototype, for example); checking hasOwnProperty in every for-var-in loop (or using a utility function like _.each); or, in theory, making your own non-enumerable properties, but this doesn't have universal browser support.

There's no way to discover non-enumerable properties in IE 8, but you can probe particular ones; for example, Date.prototype.hasOwnProperty("toString") will succeed.

The criterion of "prototype has properties" seems to exclude some real constructors.  For example, browsers come with a ton of built-in classes now, and some have no properties -- like WebGLTexture in Chrome or ActiveXObject in IE 8.  Technically, all bets are off when it comes to built-in or "host" objects -- they can have methods without having corresponding functions on the prototype -- but browsers in general seem to make things look nice.  (Here's a weird fact I discovered once, though: in IE 8, `typeof window.open === "object"`, and the same with other built-in methods.)  There are various reasons developers might define classes with no instance methods, too.  Error classes are one example.  It's also nice to be able to define "struct" classes like new Point(x,y) even if there are no methods.  EJSON makes struct classes much more useful by making them serializable, but it also requires them to have instance methods, so maybe that saves us there.

I have mixed feelings about needing to tell constructors from normal functions.  I think the heuristic will need several parts, and it will be imperfect enough that we'll need to document it front and center because people will run into it eventually if they think they're getting instanceof semantics.

I was going to propose a third argument to check -- `check(value, Object, function () { /* test */ })` -- but that means the test is not part of the pattern so you can't save it like a typedef.  I don't have any other ideas off the top of my head and I do like being able to pass a raw function.  I can see how the goal here is to make things as simple as possible for the developer and not make people convert between the two notions of "is" or call two different functions.

All I can say is go to whatever lengths you can to detect constructors.

-- David

[David Glasser]

Are we saying "does F.prototype have any fields directly" or "are
there any methods anywhere up the chain"?

Because I can totally imagine something like

var SuperClass = function () {
...
};

SuperClass.prototype.someMethod = function () {...};

var SubClass = function (x) {
// some specific initialization
};
util.inherits(SubClass, SuperClass)

-- ie, SubClass has no methods of its own, but it does have some
nontrivial constructor logic (and is differentiable from other
subclasses via instanceof)

[Andrew Wilcox]

On Wednesday, May 8, 2013 5:00:30 PM UTC-4, David Glasser wrote:

Are we saying "does F.prototype have any fields directly" or "are
there any methods anywhere up the chain"?

We can walk up the chain if we want.

But the point that not all existing or useful classes have methods makes me nervous about the heuristic idea.  If it were true that calling a constructor directly (without using `new`) did always return `undefined` (and so would fail as a predicate), I wouldn't be very worried.  But JavaScript constructors can return a value if they want to, which is used in place of the initially constructed object.  (Some people use this to return a particular subclass).  For example, calling `Meteor.Error` without `new` returns a truthy object, and so could be mistakenly used as a predicate without causing a match failure.

[Andrew Wilcox]

Here's a thought.  Suppose `registerConstructor` was used to specify a constructor function as a pattern, but we also had a heuristic that warned if an apparent constructor function was being mistakenly used as a predicate (it hadn't been registered).  Now we can be helpful and load the heuristic with all the cases we can think of (WebGLTexture defined?  add it to the list!)... without relying on the heuristic being 100% correct in all cases.

[David Greenspan]

If I were to tweak Nick/Geoff's proposal, I'd say make it so predicate functions aren't allowed to return anything typeof object.  Then we will actually never call a constructor and succeed.  The error message should point people towards using Match.InstanceOf.

The remaining ugliness is that the developer may have to be aware of the heuristic, to make sense of the world when it fails.

Having a call like registerConstructor helps, especially when you think of packages that define classes.  The package author can shield the developer from needing to write Match.InstanceOf.

In the long term, I wonder if the linker will know what's a constructor somehow, like from a jsdoc-like annotation?

-- David

On Wednesday, May 8, 2013, Andrew Wilcox wrote:

Here's a thought.  Suppose `registerConstructor` was used to specify a constructor function as a pattern, but we also had a heuristic that warned if an apparent constructor function was being mistakenly used as a predicate (it hadn't been registered).  Now we can be helpful and load the heuristic with all the cases we can think of (WebGLTexture defined?  add it to the list!)... without relying on the heuristic being 100% correct in all cases.

[Andrew Wilcox]

Aha, yes, much better to require predicate functions to return true/false.

Hmm, so a registerConstructor in addition to the heuristic (to help train the heuristic)?  Not a bad idea either.  And it wouldn't need to be "Match.registerConstructor", since other packages like controllers are interested in what is a constructor as well.

[David Glasser]

Gosh, this is starting to sound really complicated, and hard to document.

Is Match.Where(function () {}) really so hard to type? It's certainly
easier to explain.
Or what if the syntax was Match(function() {}) instead of Match.Where?

[David Greenspan]

Yeah, true.  I think we're just trying to get closer to the brevity of assertions, like `assert(x > 0)`.  I like defining types personally, and usually constraints like this have some basis in a type (`RevisionId = Match.Where(...)`), but if you can't use these types in any other way then I feel like I'm doing extra typing rather than saving typing.  Maybe the answer is you don't use check(.) for stuff like this, you just check that x is a Number and then throw a more specific error if the value is bad.

[Morten Henriksen]

I'm not sure how locked You guys are on the API. I'm just trying to follow and provide input - so I apologize in advance because I meddle.

Could we have a simple Match function two optional three arguments:

- variable/object/function to test

- validation // Types, objects or pattern functions

- options // optional, hasObject, oneOf, throwErrors ?

-> returns true/false // one could thrown new Match.Error(...); if needed?

Match(a, String);

// oneOf iterates over an array

Match(a, [String, Integer, Boolean], { oneOf: true, optional: true } );

// value can be undefined / empty or Integer

Match(a, Integer, { optional: true });

Match(a, [{a: 'Test'}, { b: 'foo' }], { oneOf: true, optional: true });

Match(a, {a: 'Test'}, { hasObject: true } );

Match(a, [{a: 'Test'}, { b: 'foo' }], { oneOf: true, hasObject: true, optional: true });

Match(a, Function); // if testing for function, should first make sure a is not instanceof Number, Date etc..

Match(a, function(a) {

return Match(a.b, { c: 'hmm'});

});

// Options would be passed down to the pattern function

Match(a, function(a, options) {

        if (options.custom)

        return Match(a.b, { c: 'custom'}, { hasObject: true, throwError: options.throwError })

        else

      return Match(a.b, { c: 'foo'}, { hasObject: true, throwError: options.throwError });

}, { throwErrors: true, custom: true });

[Morten Henriksen]

I forgot to mention an example or use of options:

We could extend the "buildin" patterns eg. Integer with options (asserts?):

Match(a, Integer, { min: 0, max: 10 }); // or have mongo like operators like: $eq, $lt, $gt, $lte, $gte, $exists, $in ...

[Andrew Wilcox]

On Wednesday, May 8, 2013 9:04:36 PM UTC-4, David Greenspan wrote:

If I were to tweak Nick/Geoff's proposal, I'd say make it so predicate functions aren't allowed to return anything typeof object.  Then we will actually never call a constructor and succeed.  The error message should point people towards using Match.InstanceOf.

Good idea!  I've updated my PR to require predicate functions to return true/false, with an error message suggesting using Match.InstanceOf or Match.registerConstructor if using a constructor.

(I'm not using a typeof object test, because constructors will usually return `undefined` when called without `new`).

I've also added `Match.InstanceOf` as requested by Geoff.

https://github.com/meteor/meteor/pull/1008

To review this proposal:

* Predicates are common in checks and this allows them to be used without being wrapped.

(If I want to use a constructor as a match pattern I only need to call `registerConstructor` for the constructor once; but if I have to wrap predicates in `Match.Where` or `Match` then I have to do that for every predicate).

* There's no heuristic in this PR.  I'm not opposed to a heuristic if someone wants to add one, but it appears that there would need to be some way to register constructors anyway for constructors that the heuristic can't detect... so it's not clear to me that the benefit of using a heuristic would outweigh the added complexity of documenting and understanding its behavior.

* In the future, constructors could be registered e.g. with a jsdoc-like annotation, and checks will continue to work as written.

On Wednesday, May 8, 2013 10:12:19 PM UTC-4, David Glasser wrote:

Gosh, this is starting to sound really complicated, and hard to document. 

I think we may be suffering here a bit from the paradox of choice :-)   I would suggest treating the heuristic idea as an independent proposal.   I note my PR already includes documentation, and I believe it is not complicated to understand or explain.

On Wednesday, May 8, 2013 11:44:52 PM UTC-4, David Greenspan wrote:

Yeah, true.  I think we're just trying to get closer to the brevity of assertions, like `assert(x > 0)`.  I like defining types personally, and usually constraints like this have some basis in a type (`RevisionId = Match.Where(...)`), but if you can't use these types in any other way then I feel like I'm doing extra typing rather than saving typing.  Maybe the answer is you don't use check(.) for stuff like this, you just check that x is a Number and then throw a more specific error if the value is bad.

I'm sorry, I'm not quite following you here...

[Andrew Wilcox]

(I'm not using a typeof object test, because constructors will usually return `undefined` when called without `new`).

No wait, you're right, falsey values are fine because the match will fail.  I updated the PR.

 

Here's a version which moves recognizing constructors out of the matching code to make it more general purpose (where it could be used by controllers, for example).

https://github.com/meteor/meteor/pull/1028

This introduces `Meteor.isConstructor` as a function which returns true for values recognized as a constructor; it is a convenient locus to add heuristics (if desired and if it seems not too confusing to explain).  `Meteor.declareConstructor` is a function to make a constructor known to Meteor which can be used as-is, or as a supplement to the heuristic for constructors not recognized by the heuristic.

[Nick Martin]

After going over this thread again and going back and forth with Geoff, I think having `check(value, UserDefinedClass)` work without a `Match.X` prefix is pretty much a hard requirement. It is parallel with the builtins, which makes for a much smoother and more explainable API.

`registerConstructor` is a good idea, but it is too heavyweight to commit to at this point. The idea that all classes in Meteor have to be registered is a big commitment. We'd want to make and maintain a whitelist of native classes across all browsers, and perhaps more importantly people packaging JavaScript libraries for use in Meteor would have another step to make a complete package.

We've got to ship something (0.6.3 coming soon!), so we're gonna go with what we have for now. If we figure out how to distinguish classes from bare functions, we can deprecate Match.Where later by simply making it be an identity function.

Cheers,

-- Nick