Remove SimpleSchema dependency

[stubailo]

It's just for argument checking, so we don't really need it.

[lukejagodzinski]

Yes if we have to make arguments checks let's just use check.

I would leave something like this:

ValidationError = class extends Meteor.Error {
  constructor(errors, message = 'Validation Failed') {
    check(arguments, /* rule */);

    super(ValidationError.ERROR_CODE, message, errors);

    this.errors = errors;
  }
};

ValidationError.ERROR_CODE = 'validation-error';

[stubailo]

@jagi actually, can we just make check take the same schema format that you and Eric might be able to agree on, and then spit out validation errors? Then I can stop using SimpleSchema to do simple argument checks.

But this comes first.

[stubailo]

To clarify, my main problem with check is:

1. I only want to learn one schema/pattern format
2. check doesn't have enough features
3. I can't share patterns between my collection schema and my argument checks

So having a standard format would fix all of this up immediately.

[lukejagodzinski]

Hmmm I think that using "schema check" for ValidationError arguments is too heavy. It's kinda wired to use SS to check arguments of ValidationError class. If there are invalid arguments it will also throw ValidationError instance. I know that check is limited but I think that we don't need complex checks here. The ValidationError class will be used mostly by package authors and they know what are they doing. Sometimes checking every possible argument is pointless and can slow down entire application.

[stubailo]

Oh I know, I'm changing it. I'm just saying why I would prefer to use just one pattern, rather than using check's format in one place, and a different one in others.

[lukejagodzinski]

Ok good!

[lukejagodzinski]

Oh you've already made changes :) hehe I was 1 minute to late with my PR :)

[stubailo]

Yeah I also took the opportunity to clean up a bunch of other junk. Thanks for the PR, anyway :P

[lukejagodzinski]

:)

[aldeed]

I agree with this change because the circular dependency already complicates things a bit (simple-schema also depends on validation-error). But I honestly would prefer to not have the check dependency either. For this tiny amount of argument checking, why not just write it out? Otherwise someone using SimpleSchema or Astronomy needs an additional dependency they aren't using.

(Granted check will likely be a dep anyway since a million other packages use it, but I can envision a future in which that is not the case.)

[stubailo]

I think basically every single part of core meteor uses check, so the future in which you can avoid having it in your app at all is far away indeed. Perhaps if we were trying to publish to npm it could be removed - thoughts?

[lukejagodzinski]

I would also remove it. As I think about the check the more appropriate place to use it is meteor methods and publications. In fact, it's what documentation is saying. Of course it's not like it's limited only for those places. However it throws Meteor.Error which is designed to be send back to the client. Does client user need to know about these errors? If there is an error it will throw standard JS exception and we will receive error anyway. And after taking a look into the console we can tell what's going on. In Astronomy I'm very often checking arguments' types and throwing the TypeError exception instead.

[stubailo]

As I think about the check the more appropriate place to use it is meteor methods and publications. In fact, it's what documentation is saying.

Yes, but in all core Meteor packages - Mongo, Accounts, etc. check is used to validate argument types in functions. What would be the benefit of using something else? You'd basically end up writing some kind of lightweight library anyway, and then you would end up with check again.

[lukejagodzinski]

Yes you're right in fact I'm using Match.test() and later throwing TypeError :). I've used to use underscore but Match.test() is easier to use and to my surprise in most cases faster. So for me it's just a problem of what error type should be throw. But it's not so big deal.

[aldeed]

My goal is to release the next SimpleSchema as a pure node package without strong deps on anything in Meteor. Using separate Meteor package or dependency injection for Tracker reactivity. I don't know yet if I will be able to achieve this.

Currently in order to do that, I would have to replicate the validation-error package as a pure node package anyway, so I could take out the check dep as part of that. However, it brings up the question: Should this official validation-error package just be moved to an npm package? I don't see anything about Meteor.Error that requires a Meteor environment.

But anyway that is why I suggest removing the check dep, because I'd like to use it in non-Meteor apps. It's true though that you'd end up needing some lightweight checker, but BTW, we have also published a pure Node version of check: https://www.npmjs.com/package/simplecheck

Of course if every pkg is using a different lightweight checker library, then it's not really lightweight anymore. :) I guess that is an argument in favor of us all adopting TypeScript, which I think we will.

[aldeed]

It would actually be really nice if ddp-server package would just send any thrown error that has clientSafe: true property set, or something similar. Then this package could work with DDP but not actually have any Meteor or DDP deps.

[stubailo]

I think it would make a lot of sense to make the main code for check live on NPM, and just have the Meteor core check package pull it in through NPM.depends or whatnot. What do you think of that?

[aldeed]

Agree