-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add push parameter checking and query installation #198
Add push parameter checking and query installation #198
Conversation
e4e44a6
to
8481e0b
Compare
cc @bnham @fantasist for review |
Please rebase after #196 is merged. |
@nlutsenko sure, will do that. |
if (typeof process !== 'undefined' && process.env.NODE_ENV === 'test') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a good way to expose internal methods for testing purpose? I follow the same way we do in JS and React repo, but I am not so confident about this. @peterdotjs @hallucinogen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have used TESTING=1
which loads testing-routes, you could re-use that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gfosco , haha, then I know why we have TESTING=1
. But it seems we have a bug here. I remember all parameters in process.env
is string so process.env.TESTING == 1
will always return false. Could you double check that?
If TESTING = 1
is just for test purpose, probably using process.env.NODE_ENV
is a better idea. It is more common use than setting random variable and we can get rid of the integer/string stuff. I will send a PR if you do not mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that sounds fine to replace TESTING with NODE_ENV. Pretty sure JS coerces "1" == 1 just fine, it's "1" === 1 that would fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting the original question: yes the check looks fine. Just make sure it's consistent across the code. In JS SDK we achieve this by caching the value in some JSON config https://github.com/ParsePlatform/Parse-SDK-JS/blob/master/src/CoreManager.js#L111
9075fe2
to
65b19cd
Compare
65b19cd
to
fe4c1cc
Compare
if (!hasExpirationTime) { | ||
return; | ||
} | ||
expirationTime = moment(new Date(body['expiration_time'])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the API, we allow expiration_time
to be either a number or a string. If it's a number, then it's the number of seconds since the unix epoch. If it's a string, it's an ISO8601 string.
So this needs to be something like:
expirationTimeParam = body['expiration_time'];
if (typeof expirationTimeParam == 'number') {
// date constructor wants ms since epoch, not seconds
expirationTime = new Date(expirationTimeParam * 1000);
} else if (typeof expirationTimeParam == 'string') {
expirationTime = new Date(expirationTimeParam);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
fe4c1cc
to
096c172
Compare
@wangmengyan95 updated the pull request. |
096c172
to
a683bef
Compare
@wangmengyan95 updated the pull request. |
body['expiration_time'] + ' is not valid time.'); | ||
} | ||
// Check expirationTime is valid or not, if it is not valid, expirationTime is NaN | ||
if (!isFinite(expirationTime)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it might be some left over code from before? expirationTime is a Date, doesn't seem like it makes sense to call isFinite on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, isFinte
can be used to check Date
object directly, since it will auto transfer Date
to number. If Date
is invalid, it will return false
, otherwise it will return true
.
LGTM, take a look at that isFinite call |
…hecking_and_run_queries Add push parameter checking and query installation
Add class ParseApp
This is the first step to implementing the push system in parse-server. In this PR, I add
Since it is not finished yet, the api will still return error.