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

Added Match.Maybe to allow optional matching with null support #6220

Closed
wants to merge 2 commits into from

Conversation

LewisW
Copy link
Contributor

@LewisW LewisW commented Feb 11, 2016

This pull request implements the proposed Match.Maybe suggested in #3876. Match.Maybe adds support for a null value being accepted as an optional value. This is necessary because DDP converts undefined values to null values.

I have not updated the docs yet, I wanted to check I was on the right track before adding something to the docs.

@stubailo
Copy link
Contributor

Looks awesome!

Please update the docs and I'll merge.

@sebakerckhof
Copy link
Contributor

Very useful.

@LewisW
Copy link
Contributor Author

LewisW commented Feb 15, 2016

Documentation updated. I wasn't sure if you wanted me to recommend it over Match.Optional or just document it as an alternative and trust Meteor developers to use it themselves when appropriate.

@stubailo
Copy link
Contributor

Merged as one commit: 7a80a8c

Thank you!

Going to update the docs to make it stand out more, and possibly say that Optional is no longer recommended.

@abernix
Copy link
Contributor

abernix commented Apr 7, 2016

@stubailo had wrote:

I don't think it would be good to silently change the behavior (of Match.Optional) because it could introduce undiscoverable security holes into people's apps.

Unfortunately, this PR did cause Match.Optional to work identical to Match.Maybe, even though it wasn't intended to. There weren't any existing tests for Match.Optional on null and none were added. Didn't find a security hole (luckily) but did find this problem. 😈

The current version is just setting var Maybe = Optional; but checking the instanceof in an unreachable else if (because of them being ===). Essentially the Match.Optional logic was not being called at all in 1.3 code, instead always using Match.Maybe.

See #6735 for more information and #6736 for the PR fix. Added a bunch of tests that prove the regression and fix too.

@stubailo
Copy link
Contributor

stubailo commented Apr 7, 2016

Oh man, that's a great catch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants