constrain what constitutes a valid Item for crud operations, and require auth #248
Conversation
i love it when a plan comes together. i've pushed up another commit that incorporates your suggestion. |
| /** | ||
| * requestOptions.owner is given priority, requestOptions.item.owner will be checked next. If neither are present, authentication.username will be assumed. | ||
| */ | ||
| function determineOwner(requestOptions: any): string { |
There was a problem hiding this comment.
i'm sure it'd be better to use a union type here instead of casting to any, but i was worried that i'd already reached my daily limit of new interfaces for this package today.
There was a problem hiding this comment.
Yeah, I think using any is fine in this case for a simple utility function.
| @@ -48,8 +57,7 @@ export interface IItemResourceRequestOptions extends IItemIdRequestOptions { | |||
| resource?: string; | |||
| } | |||
|
|
|||
| export interface IItemCrudRequestOptions extends IUserRequestOptions { | |||
| item: IItem; | |||
| export interface IItemCrudRequestOptions extends IRequestOptions { | |||
There was a problem hiding this comment.
@jgravois Why remove IUserRequestOptions? That already has a mandatory authentication property.
There was a problem hiding this comment.
to be honest, i got so excited about the lighter-weight technique i discovered for getting TypeScript to allow me to introspect authentication which may or may not be included in a request (in Esri/hub.js#35 (comment)) that i basically forgot what IUserRequestOptions was doing for us and went ahead and reinvented the 🎡.
|
it ended up being easier to address #241 and this issue holistically. its a more involved refactor, but the approach i was suggesting before just feels like a 🤕. |
|
@noahmulfinger if you can find time to review these changes in the next couple days, i'd love to include them in the looming release. |
|
@jgravois Taking a look now. |
There was a problem hiding this comment.
@jgravois This looks good to me! I had two comments/questions, but they're really minor and don't need to be addressed for merging.
| isViewOnly: boolean; | ||
| isOpenData: boolean; | ||
| isFav: boolean; | ||
| snippet?: string; |
There was a problem hiding this comment.
snippet could probably be moved to IGroupAdd since it fits in with properties that can be passed in to create a group.
| created: number; | ||
| modified: number; | ||
| protected: boolean; | ||
| } |
There was a problem hiding this comment.
What's the reason for requiring these specific properties for IItem?
There was a problem hiding this comment.
i've written the interface this way to acknowledge that these props are all guaranteed to exist (after an item has been created).
AFFECTS PACKAGES: @esri/arcgis-rest-items ISSUES CLOSED: #246
extending interfaces between categories proved to be more complicated than helpful there is already way too much complexity between what items and groups need to look like when they are created, updated and retrieved. AFFECTS PACKAGES: @esri/arcgis-rest-common-types @esri/arcgis-rest-groups @esri/arcgis-rest-items @esri/arcgis-rest-users
|
final feedback addressed (and followed by a rebase). thank you @noahmulfinger! |
resolves #246:
stops using the (unnecessary) interfaceIUserRequestOptionscreateItem()now requires an Item with 'title' and 'type'updateItem()now requires an Item with an 'id'the methods above, andremoveItem(), now all require authenticationno one loves interface soup more than @dbouwman. hopefully he can find time to review soon as @tomwayson is out until late next week.