Add support for uploading blob item's data #254

MikeTschudi · 2018-07-23T21:11:17Z

For reasonable browsers, a Blob can be converted into a File and sent up with its filename. For IE 11 & Edge, the File constructor is not available, and even if one adds the missing File properties to the Blob, it's sent up to AGOL as "blob" by these two browsers. Changing encode-form-data to use the filename in the FormData.append call gets past this behavior.

I changed the name of IItemJsonDataRequestOptions from a documentation perspective since it can be used by both the JSON and binary functions.

Working on test code.

jgravois · 2018-07-23T21:16:02Z

packages/arcgis-rest-items/src/items.ts

@@ -25,9 +25,9 @@ export interface IItemIdRequestOptions extends IUserRequestOptions {
  owner?: string;
 }

-export interface IItemJsonDataRequestOptions extends IItemIdRequestOptions {
+export interface IItemJsonRequestOptions extends IItemIdRequestOptions {


to convey that it supports both JSON object literals and blobs, would IItemDataRequestOptions be a better (re)name?

That was the intention--I erred.

jgravois · 2018-07-23T21:17:14Z

packages/arcgis-rest-items/src/items.ts

+ */
+export function addItemBinaryData(
+  requestOptions: IItemJsonRequestOptions
+): Promise<any> {


there is a lot of Promise<any> in this package, but since we know what the signature of the response will be, it'd be good to be explicit about what this method returns.

jgravois · 2018-07-23T21:19:28Z

packages/arcgis-rest-items/src/items.ts

+export function addItemBinaryData(
+  requestOptions: IItemJsonRequestOptions
+): Promise<any> {
+  const owner = requestOptions.owner || requestOptions.authentication.username;


hot off the presses (in #252). you can now use determineOwner()

const owner = determineOwner(requestOptions);

jgravois · 2018-07-23T21:22:52Z

packages/arcgis-rest-items/src/items.ts

+  requestOptions.params = {
+    ...requestOptions.params,
+    file: requestOptions.data
+  };


this is a nitpick, but i prefer to mixin user-supplied options after internal props are set.

it feels like a nice contract to explicitly prohibit overwriting what the developer passes in.

requestOptions.params = { file: requestOptions.data, ...requestOptions.params };

coveralls · 2018-07-23T21:27:16Z

Coverage remained the same at 100.0% when pulling 39a0710 on feat/binary-data-upload into be8628b on feat/binary-data.

jgravois

i just pushed up another commit with:

a couple naming tweaks
another test (to get coverage back to 100%)
two new interfaces for add/update methods to document the responses that will be returned by the server.

i'm scratching my head as to what in the hell is going on with JS Dates on the travis box right now. locally, its all 💚

new Date(2016,7,17).getTime()
> 1471392000000 // ????

edit:
i'm going to be out of the office 'til friday. hopefully this'll give @patrickarlt a chance to look at #251 on its own first. if he's happy with it, we'll merge both and ship.

MikeTschudi · 2018-07-24T03:41:21Z

@jgravois thank you for the review and improvements. Travis date puzzled me; switched to constructing date from milliseconds to see if that will work both with npm test and Travis.

MikeTschudi · 2018-07-24T23:35:31Z

@jgravois would you please review the last two commits: changing the Date construction because of the Travis problem and adding a test for encode-form-data?

jgravois · 2018-07-27T17:39:54Z

would you please review the last two commits

looks great! i'm going to try to merge/release today.

tomwayson

Other than the suggested name change this LGTM, thanks @MikeTschudi!

tomwayson · 2018-07-27T20:52:35Z

packages/arcgis-rest-items/src/items.ts

@@ -30,9 +30,9 @@ export interface IItemIdRequestOptions extends IUserRequestOptions {
  owner?: string;
 }

-export interface IItemJsonDataRequestOptions extends IItemIdRequestOptions {
+export interface IItemDataOptions extends IItemIdRequestOptions {


I would suggest naming this IItemDataRequestOptions

agreed. i introduced that typo during code review.

i'm going to rebase this branch to clean up the commit history and then merge.

actually, just realized i already used IItemDataRequestOptions in #251.

my best suggestion for this one is IItemDataAddRequestOptions.

Not to be that guy, but I would have gone w/ IItemAddDataRequestOptions

AFFECTS PACKAGES: @esri/arcgis-rest-items

…s of non-objects Fixed addItemData input type and mix-in ordering #254 Switched to using determineOwner call #254 doc response, add another test method and interface rename whoops Changed Date constructor to see if Travis behaves better #254 Added test for FormData path #254 one more interface name change

jgravois · 2018-07-27T21:29:53Z

🚀

MikeTschudi requested a review from jgravois July 23, 2018 21:11

MikeTschudi changed the title ~~⚠️ WIP: Add support for uploading blob item's data~~ WIP: Add support for uploading blob item's data Jul 23, 2018

jgravois reviewed Jul 23, 2018

View reviewed changes

jgravois force-pushed the feat/binary-data branch 2 times, most recently from 8e357cb to be8628b Compare July 23, 2018 21:27

MikeTschudi added a commit that referenced this pull request Jul 23, 2018

Fixed addItemData input type and mix-in ordering #254

f769eb1

MikeTschudi added a commit that referenced this pull request Jul 23, 2018

Switched to using determineOwner call #254

5575015

jgravois approved these changes Jul 24, 2018

View reviewed changes

MikeTschudi added a commit that referenced this pull request Jul 24, 2018

Changed Date constructor to see if Travis behaves better #254

5ba71cf

MikeTschudi added a commit that referenced this pull request Jul 24, 2018

Added test for FormData path #254

a79227d

tomwayson approved these changes Jul 27, 2018

View reviewed changes

jgravois changed the title ~~WIP: Add support for uploading blob item's data~~ Add support for uploading blob item's data Jul 27, 2018

jgravois and others added 3 commits July 27, 2018 14:14

feat(): support for adding blobs as item/data

0cfd275

AFFECTS PACKAGES: @esri/arcgis-rest-items

Add support for uploading blob item's data

257edb6

jgravois force-pushed the feat/binary-data-upload branch from a79227d to 39a0710 Compare July 27, 2018 21:17

jgravois closed this Jul 27, 2018

jgravois reopened this Jul 27, 2018

jgravois merged commit 41a540a into feat/binary-data Jul 27, 2018

jgravois deleted the feat/binary-data-upload branch July 27, 2018 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for uploading blob item's data #254

Add support for uploading blob item's data #254

MikeTschudi commented Jul 23, 2018

jgravois Jul 23, 2018

MikeTschudi Jul 23, 2018

jgravois Jul 23, 2018

jgravois Jul 23, 2018

jgravois Jul 23, 2018 •

edited

Loading

coveralls commented Jul 23, 2018 •

edited

Loading

jgravois left a comment •

edited

Loading

MikeTschudi commented Jul 24, 2018

MikeTschudi commented Jul 24, 2018

jgravois commented Jul 27, 2018

tomwayson left a comment

tomwayson Jul 27, 2018

jgravois Jul 27, 2018

jgravois Jul 27, 2018

tomwayson Jul 27, 2018 •

edited

Loading

jgravois commented Jul 27, 2018

Add support for uploading blob item's data #254

Add support for uploading blob item's data #254

Conversation

MikeTschudi commented Jul 23, 2018

jgravois Jul 23, 2018

Choose a reason for hiding this comment

MikeTschudi Jul 23, 2018

Choose a reason for hiding this comment

jgravois Jul 23, 2018

Choose a reason for hiding this comment

jgravois Jul 23, 2018

Choose a reason for hiding this comment

jgravois Jul 23, 2018 • edited Loading

Choose a reason for hiding this comment

coveralls commented Jul 23, 2018 • edited Loading

jgravois left a comment • edited Loading

Choose a reason for hiding this comment

MikeTschudi commented Jul 24, 2018

MikeTschudi commented Jul 24, 2018

jgravois commented Jul 27, 2018

tomwayson left a comment

Choose a reason for hiding this comment

tomwayson Jul 27, 2018

Choose a reason for hiding this comment

jgravois Jul 27, 2018

Choose a reason for hiding this comment

jgravois Jul 27, 2018

Choose a reason for hiding this comment

tomwayson Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

jgravois commented Jul 27, 2018

jgravois Jul 23, 2018 •

edited

Loading

coveralls commented Jul 23, 2018 •

edited

Loading

jgravois left a comment •

edited

Loading

tomwayson Jul 27, 2018 •

edited

Loading