Esri / arcgis-rest-js

compact, modular JavaScript wrappers for the ArcGIS REST API
https://developers.arcgis.com/arcgis-rest-js/
Apache License 2.0
347 stars 120 forks source link

"get Authenticated methods getItemResource defaults to read as blob" error in unit tests on Node v18 #1052

Closed gavinr closed 1 year ago

gavinr commented 1 year ago

Describe the bug

On Node v18, getting an error:

 get Authenticated methods getItemResource defaults to read as blob
  Message:
    Failed: response[readMethod] is not a function

This is happening for me locally and this is happening in GitHub Actions runners: https://github.com/Esri/arcgis-rest-js/pull/1048 (https://github.com/Esri/arcgis-rest-js/actions/runs/3735781808/jobs/6339403995)

Reproduction

  1. Install Node v18
  2. npm install
  3. npm test

Logs

Failures:
1) get Authenticated methods getItemResource defaults to read as blob
  Message:
    Failed: response[readMethod] is not a function
  Stack:
    TypeError: response[readMethod] is not a function
        at file:///C:/.../arcgis-rest-js/packages/arcgis-rest-portal/src/items/get.ts:442:29
  Message:
    Error: Timeout - Async function did not complete within 5000ms (set by jasmine.DEFAULT_TIMEOUT_INTERVAL)
  Stack:
    Error: Timeout - Async function did not complete within 5000ms (set by jasmine.DEFAULT_TIMEOUT_INTERVAL)
        at <Jasmine>
        at listOnTimeout (node:internal/timers:564:17)
        at processTimers (node:internal/timers:507:7)

607 specs, 1 failure
Finished in 25.106 seconds

System Info

Node: v18.12.0
NPM: v8.19.2

Additional Information

No response

gavinr commented 1 year ago

My best guess right now on what is causing this: fetch-mock is not putting the .blob() function on the response object here: https://github.com/Esri/arcgis-rest-js/blob/367a49c56daa07e09665e43454a80ae6600534f5/packages/arcgis-rest-portal/src/items/get.ts#L442. I'm not exactly sure why that would change in Node v16 vs Node v18. Maybe it's because Node 18 includes fetch globally.

We are on v5 of fetch-mock, and the latest is v9. We think that upgrading to the latest (v9) version of fetch-mock may resolve this issue (#1053).

patrickarlt commented 1 year ago

@gavinr it occured to be when looking at #1047 that this is probably happening because of https://github.com/Esri/arcgis-rest-js/blob/main/packages/arcgis-rest-request/src/request.ts#L408-L412 where we default to using globalThis.fetch if it is available instead of always using node-fetch.

The issue might be that we are mixing and mathcing by using node-form-data in https://github.com/Esri/arcgis-rest-js/blob/main/packages/arcgis-rest-request/src/utils/encode-form-data.ts#L22 but passing it to the native fetch.

I think we could probally fix this by always using node-fetch from @esri/arcgis-rest-fetch in https://github.com/Esri/arcgis-rest-js/blob/main/packages/arcgis-rest-request/src/request.ts#L408-L412 and probally have better Node 18 compatibility too.