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

fix(IItem): make id and owner the only required properties of IItem #184

Merged
merged 2 commits into from
May 10, 2018

Conversation

jgravois
Copy link
Contributor

@jgravois jgravois commented May 2, 2018

AFFECTS PACKAGES:
@esri/arcgis-rest-common-types

ISSUES CLOSED: #171

@jgravois jgravois requested a review from noahmulfinger May 2, 2018 23:20
Copy link
Member

@tomwayson tomwayson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, I agree w/ travis on this one - can't make id required b/c when you're adding items you don't have one of those.

@jgravois
Copy link
Contributor Author

jgravois commented May 7, 2018

whoops.

i didn't see any errors locally because i didn't run npm run build after making the change 🤕.

once everyone's happy i'll rebase and reword the commitizen message.

@coveralls
Copy link

coveralls commented May 7, 2018

Coverage Status

Coverage remained the same at 99.43% when pulling b213fbd on patch171 into 20296ab on master.

@Esri Esri deleted a comment from coveralls May 7, 2018
@Esri Esri deleted a comment from coveralls May 7, 2018
@noahmulfinger
Copy link
Contributor

noahmulfinger commented May 7, 2018

@jgravois Looks like fixing the updateItem issue in #171 requires a more fine-grained approach. We should probably just make all IItem properties optional and then extend IItem whenever a function requires certain properties.

@tomwayson
Copy link
Member

Good idea @noahmulfinger - I suggest something likeIUpdateItemParams which inherits from IItem

@jgravois
Copy link
Contributor Author

jgravois commented May 8, 2018

We should probably just make all IItem properties optional

Agreed.

a perfect example of why is createItemInFolder().

it ignores item.owner entirely in favor of requestOptions.owner (or requestOptions.authentication.username)

i've tacked on another commit which includes the value in the decision tree (with more precedence than either of the other two sources).

if y'all are up for it, my preference would be to add a deprecation warning regarding requestOptions.owner and yank it entirely in v2.0.0

@jgravois jgravois added this to the v1.2.0 milestone May 9, 2018
@jgravois
Copy link
Contributor Author

jgravois commented May 9, 2018

after chatting w/ @tomwayson briefly i'm convinced that the correct order of precedence is

  1. requestOptions.owner
  2. requestOptions.item.owner
  3. requestOptions.authentication.username

i did a little manual testing and confirmed that regardless of whether item.owner is omitted in the request or contains a value that doesn't match the url route, the REST API will overwrite with the appropriate JSON. i'm happy about that.

@jgravois
Copy link
Contributor Author

ready for re-review (ideally from @tomwayson)

jgravois added 2 commits May 10, 2018 09:49
AFFECTS PACKAGES:
@esri/arcgis-rest-common-types

ISSUES CLOSED: #171
previously we looked to an explicit owner constructor option and authentication.owner only

AFFECTS PACKAGES:
@esri/arcgis-rest-items

💅

doc improvements
@@ -34,10 +34,6 @@ export interface IItemJsonDataRequestOptions extends IItemIdRequestOptions {
* JSON object to store
*/
data: any;
/**
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed a duplicate property present in the interface being extended.

@jgravois jgravois merged commit 9c508f2 into master May 10, 2018
@jgravois jgravois deleted the patch171 branch May 10, 2018 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants