Thanks, Avi - that’s a great point where this deviates from regular methods in a potentially unexpected way.
You have two options:
Set up a simple-schema instance
Define a custom validate function
In the Meteor Guide we’re basically taking the stance that you have to learn simple-schema to build any Meteor app - it’s used for forms, methods, collection schemas, etc.
We could also allow a matchPattern option, but the problem with check is that it throws very poorly formatted errors. I think we’re moving in the direction of gradually eliminating check for this and other reasons.
Do you think having clearer errors about your argument validity is a good reason to learn simple-schema?
Good idea to make method invocation easier. And I also appreciate the hints to the undocumented features throwStubExceptions and returnStubValue. Wasn’t aware that these two even existed…
As you are using SimpleSchema for validation, will this also work if audit-argument-checks is activated, i.e. is is calling check() internally?
As a side-note: Although I am using SimpleSchema myself, I don’t think it’s a good idea to tie this new method invocation syntax to this package. Instead, I would let the user define an object containing “check” clauses, where SimpleSchema can also be used (I’m doing this quite often).
I may be completely off-base here - but wouldn’t it be better for the guide to explain someone the problem you’ve identified and then teach them how to assemble this kind utility? And then maybe go on to say, “moving forward, we’ll follow this pattern.”
Also - maybe good to name it something other than method. It would make me think that its a package to write methods, and at first I thought that Method’s were being exported as a separate package. Perhaps structuredMethods or something
I think this is an improvement over the old API but personally my pain point was having them scattered about the app. This is really a developer/team issue but they tend to be placed all over the app and it’s hard to find them without grepping first.
I’ve also never cared for the API and I almost always setup a thin facade for my “Model” meteor methods… which also works out if you’re connecting to another DDP server to call those.
Here’s what i’d like to see but this is very opinionated and geared towards maintainability. All units are very easy to test and very easy to trace where to look for it, as the module paths are listed.
// routes.js
// define entry point for method and compose optional schema and it's function
// other serverside routes can go here too
get("/posts/:id", .....);
// destructure for less verbosity
const {Sch} = Schema.Methods;
method('Post.insert', Sch.Post.insert, Models.Post.insert);
method('emailNewUser', Sch.emailNewUser, Services.Email.newUser);
method('emailAdminUser', Services.Email.adminUser); // no schema
```
// schemas/methods.js
// the model and services should prob. be in separate submodule & files in real life
// eg. Schemas.Models.Post and Schemas.Services
To start, thank you guys for taking a look at this, I really appreciate all of the insightful feedback!
Yes, this will work with audit-argument-checks. Unfortunately, it won’t work with check-checker, but that’s no longer necessary because with this package it will be impossible to forget to validate arguments.[quote=“waldgeist, post:5, topic:13833”]
Instead, I would let the user define an object containing “check” clauses, where SimpleSchema can also be used (I’m doing this quite often).
[/quote]
I think this is a great idea and also speaks to @avital’s concerns above. In the interest of easier migration, since people might already have a ton of methods using check, I think we should add a checkPattern option that can be used instead of schema. However, I think in the long term standardizing around a more feature-complete schema definition language like simple-schema would be better; like I said above, check has a lot of problems and there’s no point in having two separate things if simple-schema has more features and a lot of people are already using it.
Would this be satisfied by explaining all of the stuff and then suggesting people use this package (or one like it) or do you mean that we should just teach people to use the boilerplate every time?
I agree - ValidatedMethod could be a better name, since that’s the primary focus of the API.
The Meteor Guide is going all-in on simple-schema, since it has benefits in many places - argument validation, schema checking, form generation, and more. It’s the most complete solution available today, possibly in the whole JavaScript ecosystem. I think we should move away from using one package (check) for validating method arguments, and another (simple-schema, astronomy) for validating collection schemas.
IMO this is one of the places where a commitment to a standard format could have huge benefits for everyone - if we could all be using one JS schema format to interface between different packages. For example, if you could have lots of different form packages that all know how to validate against simple-schema, etc.
I think step 1 is standardizing around it, step 2 is finding the biggest pain points and fixing them. Right now, it seems like it’s a good idea to create yet another schema builder for Meteor, I’d rather be in a world where people instead submit PRs to simple-schema. Have you considered helping out aldeed by fixing some of the bugs yourself?
I think PRs are actually more likely to be accepted into simple-schema than Meteor core at the moment, so not building on top of a “core” library sets us up for faster iteration.
I think this will be much more easily addressed now that methods can be represented as JS objects rather than magic strings. Basically, you can tell where a method is defined by its namespace, and in Meteor 1.3 you can put methods inside a module.
We’re planning on recommending that people split their apps into modules (that often closely map to collections) so you’d hang your methods off of a common namespace, like Lists.methods. Now, you just need a list of the collections/modules of your app, and you can easily iterate over them to get a list of all of your methods.
Do you think it’s a good idea to have the schema of a method and the implementation in a different file? I feel like that would defeat the self-documenting aspect - if they are in the same place, you can easily see what argument types you should be expecting.
This package supports the use case of calling the method implementation separately from the validation, etc, by calling method._execute, which makes for easy testing; that’s farther down in the README.
Action items:
Consider renaming the package to mdg:validated-method to make it clearer that this is a wrapper around methods, not some sort of package moving the implementation of methods outside of the meteor/meteor repository. https://github.com/meteor/method/issues/2
Add an option to specify a checkPattern or matchPattern (not sure which name is better!) to make it easier to use this package without simple-schema and provide an easier migration path for people who are already using check to validate their arguments. https://github.com/meteor/method/issues/3
It’s the most complete solution available today, possibly in the whole JavaScript ecosystem.
Well, if the other thread about Blaze 1 -> 2 and React taught us anything, this statement can lend itself to replacement such as “the javascript ecosystem has accepted xyz as its schema validation standard”. In fact, there already is http://json-schema.org/ which is more likely to - perhaps even has - evolve into a standard.
I get the full-on-simple-schema approach within the guide. It is a guide and is supposed to be about one of the ways of doing things while letting the readers know that there are others and especially some worth metioning.
But this, taking a 3rd party package and making it a core part of something that is likely to find its way into core! It does not look like a move that embraces an ecosystem, (virtually) forcing all meteor apps to use simple-schema. There are so many things that could go horribly wrong here, if MDG does not fold simple schema in actual official core.
And suggesting "PR"s while heated arguments still continue on the community management/involvement front is just a knee-jerk move.
I’m all happy with your proposals on this API. It does seem much, much better. It just is not reliable from a business perspective.
While in the short term, I think simple-schema is the only thing we can adopt, I think there’s only one possible long-term sustainable answer to this: We need to either make simple-schema a JS standard, or adopt the standard thing if one exists.
I think we need to go in incremental steps here - moving to JSON schema right now would be quite a monumental undertaking and would probably set back the Meteor guide project many months. Once we have written down all of the requirements for a JavaScript schema as encoded by the guide, we should be able to better evaluate the available options.
Work with @aldeed to publish simple-schema on NPM and promote it to the wider JavaScript ecosystem
Switch to JSON schema, with a transition path (actually there is already a way to generate a SimpleSchema from a JSON schema, but I can’t find it at the moment)
I guess my point was exactly that MDG isn’t really great at moving fast and accepting PRs right now, so it actually might be better to not rely on our ability to rapidly improve something like a schema package. In the ideal world in the long term MDG would certainly be maintaining something as important as a schema package. I’m trying to work with what I have.
I thought that embracing a community package and relying on it would be a very ecosystem-embracing move; do you mean that embracing means that we should maintain the package in this context?
If validating arguments is done for security purposes then clearer errors shouldn’t matter (and that may even be a liability).
If validating arguments is done as an aid during development to help with developer mistakes then presumably most times there’s access to the server console and nicely formatted errors don’t matter much.
You’re missing displaying helpful errors if the method is called from user input - so for example if the user fills out a form, they would want to know that their input failed because a certain field is too long.
I’m not sure that the client knowing what type a certain argument is supposed to be can be considered a security vulnerability - in some sense, the API between your client and the server can’t be a point of security since anyone can de-minify your client code and simply read how your methods are called.
Although if there is a good reason to hide the errors in production we could add an option to hide errors from the client.
Yes, the form example is a good one. So how would that work? Is there a way on the client to capture the method validation errors so that they could be displayed? Or is that a different pattern?
Thanks for taking the time to present a clearer/broader picture.
Let me first reiterate that I am assuming this is a piece of work that’s going to lose the mdg: prefix and end up part of core, and I’m building my arguments upon this assumption.
As we’ve seen with the Blaze discussion, a major/vocal percentage of the community does not like it when something is standardized and then deprecated. And that such decisions should not be spur of the moment things, well thought out, planned and executed for long term sustainability.
Whatever incremental step we take here, it has to be supported in the long term and it would also pose as a stance. The even more monumental undertaking would be “hey ss is great and we all asked you to switch to it but you know, xxx is now the js standard so everyone’s to switch again”.
If you really want to improve on and push SS as a javascript standard (I wish MDG had the same enthusiasm for Blaze) please first propese a draft plan on how you’d like to achieve it. How we could help. How realistic it is from the broader JS perspective.
So if you are going to work with @aldeed, please do so, and in fact make that an official and public statement. Don’t depend on “possible future PRs” which you or aldeed may or may not find the time to look into. You say MDG may not be fast enough to accepting PRs. Well, who guarantees the opposite for aldeed?
I’m all for the well intended push forward, but Meteor is now 1.x and stability and long term support are important aspects of the platform.
My remark about not embracing the ecosystem is about shutting out SS alternatives. So if one likes the design decisions of astronomy or wants to go with json schema, it would just mean doubling the efforts by repeating code that already does the same as SS within methods. Noone wants to write the same code twice. Hence the end of SS alternatives, the “not embracing” remark.
So I guess, I do support the two options you mention, go with SS and make it a standard, or search for the standard, or the closest thing to a standard. The problem is, you have already chosen, so there actually is no choice left here.
Anyway, I still love that you took the initiative and time to fix methods and by the looks of it, it is going to end up a huge improvement over its current form.
JSON Schema seems to be the closest thing to a “standard” right now, but I think it’s a long way away from being that, TBH. I don’t think it really achieves what we need it to, in terms of features, and SS is very much part of the Meteor community already, so blessing it is way less impactful.
I’m not sure how having lots of options out there without any clue of which one you “should” use helps with code duplication though? I mean, if we’d been quicker to embrace SS, then I would guess Astronomy (which does a lot more than just schema validation) would probably have used SS for its schemas, and we’d be in a better place right now (from a duplication point of view).
(Part of the idea of this work is to make a common format for validation errors that isn’t schema specific – please open issues / PRs against https://github.com/meteor/validation-error for this!)
Subclassing mdg:method to take a different type of schema and do the same thing that we do with SimpleSchema (throwing a ValidationError) seems pretty trivial – so I don’t think we are “shutting out alternatives”, except if you mean choosing something to recommend in the guide and elsewhere; I think the consensus is that it’s a very good idea for us to do this.
What I said is that I wouldn’t have had to create simple-schema/collection2 if astronomy existed back then. I did create it, though, and tons of people rely on it and so do many of my production apps, so I have no plans to stop supporting it until people stop using it. I’m working on v2 now, part of which will be speed improvements and moving as much of it as possible to a pure node package.
To be clear, are you saying that from hence-forth I must use simple-schema to build Meteor apps? Or are you saying this is just considered best-practices by MDG?
Thoughtful, and I like the spirit of it generally! Still, Promises are part of core now, and they’re not used, that’s my only critique. Can you omit the callback parameter and get a Promise ? If not, is there any guarantee of whether the callback is invoked sync or async, or multiple times, for the simulation, and the eventual real ?
Ah I was hoping you would comment! What do you think is the best way to integrate promise support here? I feel like it’s not yet a good time to switch to always returning a promise, and doing it only sometimes seems weird. Some options:
Add support for mixins, have a promise mixin that wraps call
Make a subclass class PromiseMethod extends Method that wraps call
Add a new method callAsync or callPromise
As for the callback, it has the same semantics as Meteor.call - is there some case where that invokes the callback twice, or synchronously? This wrapper could be a good place to fix that, if so.
