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

Match.Optional should accept null in addition to undefined #3876

Closed
stubailo opened this Issue Mar 6, 2015 · 13 comments

Comments

Projects
None yet
@stubailo
Copy link
Contributor

stubailo commented Mar 6, 2015

Checking arguments to methods becomes quite silly otherwise.

First of all, arguments that are passed in as undefined are converted to null by DDP, so you can't use Match.Optional in its current state to validate missing arguments in methods on the server.

Second, even if you used Match.OneOf(null, <type>), it still wouldn't work because the method stub on the client actually does get undefined.

Perhaps we should reconsider our code style guidelines of setting things to null instead of undefined, or modify our default tools to accept null as meaning "missing value".

@meonkeys

This comment has been minimized.

Copy link
Contributor

meonkeys commented Sep 1, 2015

+1

@rijk

This comment has been minimized.

Copy link

rijk commented Sep 10, 2015

+1

Sashko’s post did give me an idea for a workaround: Match.OneOf(undefined, null, <type>). But most dev friendly IMO would be to (silently) match null it in Match.Optional. Especially since the conversion to null is done under the covers as well.

@voidale

This comment has been minimized.

Copy link

voidale commented Sep 29, 2015

+1

1 similar comment
@panphora

This comment has been minimized.

Copy link

panphora commented Oct 21, 2015

+1

@mjesuele

This comment has been minimized.

Copy link

mjesuele commented Nov 10, 2015

This just bit me as well, and is rather unintuitive behavior. +1!!

@acemtp

This comment has been minimized.

Copy link

acemtp commented Nov 24, 2015

+1!!!

@Jank1310

This comment has been minimized.

Copy link

Jank1310 commented Jan 22, 2016

+1

Workaround for matching undefined, null, and an unset key: Match.Optional(Match.OneOf(undefined, null, String)).

@LewisW

This comment has been minimized.

Copy link
Contributor

LewisW commented Feb 9, 2016

+1

@stubailo

This comment has been minimized.

Copy link
Contributor Author

stubailo commented Feb 9, 2016

I would accept a pull request for a new call, Match.Maybe, which has the correct behavior - accepts undefined, null, and the passed value.

Then we can deprecate Optional. I don't think it would be good to silently change the behavior because it could introduce undiscoverable security holes into people's apps.

If someone acts fast, we can get this into Meteor 1.3!!!

@stubailo

This comment has been minimized.

Copy link
Contributor Author

stubailo commented Feb 16, 2016

Fixed this by adding Match.Maybe. Going to make the docs recommend that instead.

@mitar

This comment has been minimized.

Copy link
Collaborator

mitar commented Feb 24, 2016

We developed those extensions: https://github.com/peerlibrary/meteor-check-extension

We have Match.OptionalOrNull to address this issue.

@lirbank

This comment has been minimized.

Copy link

lirbank commented Mar 12, 2016

A simple workaround, for pre 1.3, is to substitute Match.Optional(type) with Match.OneOf(null, undefined, type) - works on both client and server.

@maxenceC

This comment has been minimized.

Copy link

maxenceC commented Mar 16, 2016

@lirbank thank you for the workaround !

abernix added a commit to abernix/meteor that referenced this issue Apr 7, 2016

Fix `Match.Optional` to work as it did previously in Meteor 1.2
`Match.Optional` is still only supposed to "pass" if the value is `null`
or the specified type.  The new `Match.Maybe` allows `undefined` or
`null` in addition to the specified types. `Match.Optional` is on the
track toward deprecation, however to not break existing code it was
*supposed* to stay working the same as before. (per meteor#3876).

There weren't tests in place to make sure that `Match.Optional` kept
working the same, and the code didn't actually make it keep working the
same.  Hopefully extra tests will make this better.

`.Maybe` has some additional bugs, but should be addressed separately
(see meteor#6271)

Fixes meteor#6735

abernix added a commit to abernix/meteor that referenced this issue Apr 7, 2016

Fix `Match.Optional` to work as it did previously in Meteor 1.2
`Match.Optional` is still only supposed to "pass" if the value is `null`
or the specified type.  The new `Match.Maybe` allows `undefined` or
`null` in addition to the specified types. `Match.Optional` is on the
track toward deprecation, however to not break existing code it was
*supposed* to stay working the same as before. (per meteor#3876).

There weren't tests in place to make sure that `Match.Optional` kept
working the same, and the code didn't actually make it keep working the
same.  Hopefully extra tests will make this better.

`.Maybe` has some additional bugs, but should be addressed separately
(see meteor#6271)

Fixes meteor#6735

benjamn added a commit that referenced this issue Apr 11, 2016

Fix `Match.Optional` to work as it did previously in Meteor 1.2
`Match.Optional` is still only supposed to "pass" if the value is `null`
or the specified type.  The new `Match.Maybe` allows `undefined` or
`null` in addition to the specified types. `Match.Optional` is on the
track toward deprecation, however to not break existing code it was
*supposed* to stay working the same as before. (per #3876).

There weren't tests in place to make sure that `Match.Optional` kept
working the same, and the code didn't actually make it keep working the
same.  Hopefully extra tests will make this better.

`.Maybe` has some additional bugs, but should be addressed separately
(see #6271)

Fixes #6735
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.