refactor(feature-service): update signatures #214
Conversation
| @@ -83,22 +89,11 @@ export interface IQueryFeaturesParams extends ISharedQueryParams { | |||
| quantizationParameters?: any; | |||
| returnCentroid?: boolean; | |||
| resultType?: "none" | "standard" | "tile"; | |||
| // TODO: is Date the right type for epoch time in milliseconds? | |||
| historicMoment?: Date; | |||
| historicMoment?: number; | |||
There was a problem hiding this comment.
I think my idea was our API would take in dates, but convert them to epoch time (numbers) before sending the requests. That said, I didn't implement it. 😄
So I agree w/ this change. We can take in numbers for now and then later add the option to take dates (historicMoment?: number | Date).
| /** | ||
| * Layer service url. | ||
| */ | ||
| url: string; | ||
| objectIds?: number[]; | ||
| relationParam?: string; | ||
| // NOTE: either time=1199145600000 or time=1199145600000, 1230768000000 | ||
| time?: Date | Date[]; |
There was a problem hiding this comment.
I think we should change this to number | number[] as well to be consistent w/ historicMoment.
| // mixin, don't overwrite | ||
| options.params.objectIds = requestOptions.deletes; | ||
|
|
||
| return request(url, options); | ||
| } | ||
|
|
||
| function appendCustomParams( | ||
| oldOptions: IQueryFeaturesRequestOptions, |
There was a problem hiding this comment.
Should both of these be union types? IQueryFeaturesRequestOptions | IAddFeaturesRequestOptions | IUpdateFeaturesRequestOptions | IDeleteFeaturesRequestOptions
There was a problem hiding this comment.
They should, but this would require adding [key: string]: any; to all four interfaces to avoid TypeScript complaints.
Element implicitly has an 'any' type because type 'IQueryFeaturesRequestOptions | IAddFeaturesRequestOptions | IUpdateFeaturesRequestOptions | IDele...' has no index signature. (7017)
There was a problem hiding this comment.
actually, I guess just oldOptions should be that union type, is that what you tried?
There was a problem hiding this comment.
exactly. my current approach is definitely incorrect, but for some reason it doesn't bother TypeScript (or me).
we could also:
- append the generic key to each of the interfaces we'd like to union (not DRY, but that's not a big deal)
- create and export a generic
ICrudRequestOptionsinterface and have each of the ones we'd like to union extend that (this would require it to appear in the doc, which feels like overkill) - create and not export a generic
ICrudRequestOptionsinterface and use it to declare the type to the utility method without extending the existing interfaces to inherit from it (which isn't really any better than what i'm doing now). - just do this:
export function appendCustomParams(
oldOptions: { [key: string]: any; },
newOptions:
| IQueryFeaturesRequestOptions
| IAddFeaturesRequestOptions
| IUpdateFeaturesRequestOptions
| IDeleteFeaturesRequestOptions
) { /**/ }There was a problem hiding this comment.
for some reason it doesn't bother TypeScript (or me).
sorry, it bothers the shit out of me. It's not you, it's TS.
I suspect the problem is the [key: string]: any in IQueryFeaturesRequestOptions is what causes the Element implicitly has any error when you use a union type for oldOptions. Can we remove that?
Also, if you don't want to deal w/ this, just LMK and I'll pull and try to reason through it myself.
There was a problem hiding this comment.
Nice work @jgravois.
I made a couple of minor suggestions.
I think this achieves what @patrickarlt suggested in #137 (comment), but I think he should review and weigh in as well.
|
One other thing, should we add links to the REST API docs for each of the CUD request options interfaces the same way I did for the query request options. |
|
Hm... I'm confused that link doesn't make its way to the docs: https://esri.github.io/arcgis-rest-js/api/feature-service/IQueryFeaturesRequestOptions/ |
There was a problem hiding this comment.
When i was addressing @tomwayson's excellent feedback, i also went ahead and split out the source code into distinct files to make it more legible going forward.
I'm confused that link doesn't make its way to the docs: http://esri.github.io/arcgis-rest-js/api/feature-service/IQueryFeaturesRequestOptions
this was just because of the newline (and fixed now)
jgravois
force-pushed
the
chore/feature-service-params
branch
from
55b000c to
e84da39
Compare
…p leaning on params directly AFFECTS PACKAGES: @esri/arcgis-rest-common-types @esri/arcgis-rest-feature-service
jgravois
force-pushed
the
chore/feature-service-params
branch
from
e84da39 to
dd1e4c7
Compare
to stop leaning on
paramsdirectlyAFFECTS PACKAGES:
@esri/arcgis-rest-common-types
@esri/arcgis-rest-feature-service
addresses @patrickarlt's #137 (comment)
this is one rare instance in which it will 'just work' if developers don't update their code.