Touch up features endpoint - #748
Conversation
a4ecd36 to
8c04c67
Compare
|
@drew-gross updated the pull request. |
|
@drew-gross Be aware - I changed few things in #747 - it uses a slightly different logic for FeaturesRouter. Shouldn't impact this big time though. |
| return Promise.resolve({ | ||
| response: { | ||
| results: [getFeatures()] | ||
| results: getFeatures() |
There was a problem hiding this comment.
Why do we need results here as another nester layer?
It's going to surface in the response as { results: { push: .... } }.
There was a problem hiding this comment.
Results is standard across almost all our endpoints
There was a problem hiding this comment.
Also suppose there were a feature named message and another named code, it would be impossible to determine success vs. failure
There was a problem hiding this comment.
Yes, but not for a single object, and only in case where there could be more than a single result.
There was a problem hiding this comment.
That is more important in eg. /schemas where you could legitimately create a class named code and another named message
There was a problem hiding this comment.
Can we do result:{} or features:{} in this case? results doesn't really surface the truth about what you get back.
There was a problem hiding this comment.
In regards to code/message - we shouldn't be relying on keys in the body for success/failure, but on http status code. If >=400 - then the request failed and any info in the body is describing the error.
There was a problem hiding this comment.
Eh, if we are not going to follow the standard exactly I would rather go back to just using a raw object with no nested layer.
There was a problem hiding this comment.
Raw object without a nested layer - sounds great.
I am not sure whether results is a standard at this point, since it's used roughly for 2 endpoints only. Queries and schema. :)
8c04c67 to
364c7de
Compare
|
@drew-gross updated the pull request. |
|
Woooo now the dashboard can see the parse server version |
|
@drew-gross updated the pull request. |
|
nice! I thought you wanted to send it as |
| }, | ||
| }) { | ||
|
|
||
| setFeature('serverVersion', parseServerPackage.version); |
There was a problem hiding this comment.
how about we use a metadata field to hold the version info. it feels odd to treat it as a feature like the others
id like the payload to be something like:
{
metadata: {}
features: {}
}
its prob out of scope so i can make the changes in a later pr
There was a problem hiding this comment.
I like this idea. I will do that.
There was a problem hiding this comment.
Then we can include things like installed packages, adapters and their version, and so on
|
@peterdotjs my and @nlutsenko went back and forth in comments that GitHub has now hidden and landed on no |
|
lgtm. let me know about your thoughts on having |
|
@drew-gross updated the pull request. |
1 similar comment
|
@drew-gross updated the pull request. |
6f05c90 to
a56fe0f
Compare
|
@drew-gross updated the pull request. |
4 participants
As discussed, remove unused stuff and disable config and logs endpoint. Also this updates the format of the logs featureset to account for the new parameters :)