arcgis-rest-fetch / arcgis-rest-form-data is missing "main" in package.json

[b00tsy]

When building packages with pkg it is assumed, that property "main" is defined in package.json (which it is in arcgis-rest-portal / arcgis-rest-request / arcigs-rest-auth: eg.: "main": "dist/cjs/index.js")

Probably the structure of arcgis-rest-fetch / arcgis-rest-form-data differs and maybe no cjs version exists, which is why no "main" property is defined.

But if a cjs version existed it would be awesome if it was specified in "main"...

[sandromartis]

I'm currently trying to migrate from v3 to v4 and getting the following errors when running our Jest (ts-jest) test suite:

Cannot find module '@esri/arcgis-rest-form-data' from 'node_modules/@esri/arcgis-rest-request/dist/cjs/utils/encode-form-data.js'

Require stack:
  node_modules/@esri/arcgis-rest-request/dist/cjs/utils/encode-form-data.js
  node_modules/@esri/arcgis-rest-request/dist/cjs/request.js
  node_modules/@esri/arcgis-rest-request/dist/cjs/index.js

Could this be related? We are using arcgis-rest-js in node.js.

[ReneU]

@sandromartis I was running into the same issue during a migration. Adding this to the Jest configuration fixed the issue:

"moduleNameMapper": {
  "@esri/arcgis-rest-(fetch|form-data)": "<rootDir>/node_modules/@esri/arcgis-rest-$1/browser-ponyfill.js"
}

You might have to change the file to node-ponyfill.js if you're running things NodeJS

[sandromartis]

@ReneU Thanks Rene, that helped, I'm one step further :)

[sandromartis]

Not sure I fully understand what's going but bumping Jest and Typescript also solved the issue for me. The moduleNameMapper does not seem to be needed with the latest Typescript and Jest versions.

[patrickarlt]

There are 3 separate issues here.

1. pkg does not support Node JS style module resolutions and only supports a main field in package.json.
2. Jest prior to v28 didn't support conditional exports and only supports ESM modules behind a flag.
3. Jest (and potentially Babel) have issues with dynamically importing node-fetch (i.e. the import("node-fetch")) when running as CJS node.

I'll address each of these separately.

---

pkg does not support Node JS style module resolutions and only supports a main field in package.json.

The lack of a main field in the package.jsons was deliberate. The idea was to force modern module resolution rules from Node which would use exports rather then main. Many bundlers also use main for browser packages so there is no clear concenus as to if main should be CJS/ESM or Node/Browser only. Generally main is CJS modules Node but webpack includes main in its lookup fields for browsers by default however since we also define browser AND module this won't be hit.

I thought I also removed the main fields from the other packages for the 4.0 release but clearly did not. I think it would be OK to add main as CJS modules for Node to @esri/arcgis-rest-fetch and @esri/arcgis-rest-form-data to resolve this issue because it is a bug.

This does have the potential to break some things if people were using older versions of bundlers that either didn't support conditional exports or were looking at main but as it is now if your bundler is only looking at main and not conditional exports it is broken.

---

Jest prior to v28 didn't support conditional exports (for ESM or CJS) and only supports ESM modules behind a flag.

Jest only added support for conditional exports in jest@28.0.0. If you are using Jest prior to Jest v28 then Jest was likely looking for the main field which was missing. @ReneU solved this by forcing the resolution with moduleNameMapper and @sandromartis solved it by upgrading Jest.

---

Jest (and potentially Babel) have issues with dynamically importing node-fetch (i.e. the import("node-fetch")) when running as CJS node.

Specifically once you are running Jest 28+, jest will properly load node-ponyfill.js which attempts to load node-fetch. Since node-fetch is ESM only and we are in a CJS file we have to load it dynamically like import("node-fetch") which runs into a segmentation fault error in Node.

---

The solution to the first 2 issues to to add main into @esri/arcgis-rest-fetch and @esri/arcgis-rest-form-data. This might cause some breaking changes for some users but will largely be a compatibility win since MOST things expect this to be CJS modules for Node.

The only way to solve the final issue with the segmentation fault would be to not use dynamic imports in Common JS modules. This would require either:

1. Switch back to node-fetch@2 internally. This isn't particularly desirable since node-fetch@3 gives us quite a few new features and generally aligns better with the eventual direction of built-in fetch in Node.
2. Split @esri/arcgis-rest-fetch into 3 packages one using node-fetch@3 (for ESM) and the other using node-fetch@2 (for CJS) and another package to conditionally load those packages.

The only other things to do would be:

• Wait for the V8 team to fix the seg fault bug but it isn't fixed yet and once fixed would only appear in a future version of Node which would be at the earliest Node 20 in April 2023. Jest users would still have to upgrade to the latest Node version to get the fix.
• Do nothing. People who encounter the segmentation fault error need to switch their apps to ESM and use Jests ESM support.

Right now we only have confirmed that this impacts people using Jest it might impact people using Babel (based on the issue in https://github.com/nodejs/node/issues/35889). So the question is this:

Is it worth it to split @esri/arcgis-rest-fetch as described above to avoid the dynamic import so that we can fix Jest support?

[dbouwman]

Oof. I'm not sure I'm qualified to read this let alone have an opinion on how to move forward. If splitting the package solves the issue then that seems reasonable.

[tomwayson]

The solution to the first 2 issues to to add main into @esri/arcgis-rest-fetch and @esri/arcgis-rest-form-data. This might cause some breaking changes for some users but will largely be a compatibility win since MOST things expect this to be CJS modules for Node.

I think we should do this.

Re: final issue, I'm wondering about:

Do nothing. People who encounter the segmentation fault error need to switch their apps to ESM and use Jests ESM support.

Do they need to switch their apps or just their test files? Looking at Jest's ESM support it still looks pretty experimental. Still, I'd say we should consider doing nothing for now and seeing how many people are affected by this, b/c the other options, going back to node-fetch@2 either entirely or in a parallel (split) package sounds pretty bad.

[BananaGlue]

Thanks for pointing that out. Actually to problem for the vercel pkg issue seems to be dependent on external dependencies which have been migrated to ESM as described above: node-fetch, fetch-blob and formdata-polyfill, not sure, yet, what the issue with data-uri-to-buffer is.

pkg -t node16-linux-x64 .
> pkg@5.8.0
> Warning Failed to make bytecode node16-x64 for file /snapshot/restjs/node_modules/node-fetch/src/index.js
> Warning Failed to make bytecode node16-x64 for file /snapshot/restjs/node_modules/node-fetch/src/body.js
> Warning Failed to make bytecode node16-x64 for file /snapshot/restjs/node_modules/node-fetch/src/headers.js
> Warning Failed to make bytecode node16-x64 for file /snapshot/restjs/node_modules/node-fetch/src/request.js
> Warning Failed to make bytecode node16-x64 for file /snapshot/restjs/node_modules/node-fetch/src/response.js
> Warning Failed to make bytecode node16-x64 for file /snapshot/restjs/node_modules/node-fetch/src/errors/abort-error.js
> Warning Failed to make bytecode node16-x64 for file /snapshot/restjs/node_modules/node-fetch/src/errors/base.js
> Warning Failed to make bytecode node16-x64 for file /snapshot/restjs/node_modules/node-fetch/src/errors/fetch-error.js
> Warning Failed to make bytecode node16-x64 for file /snapshot/restjs/node_modules/node-fetch/src/utils/get-search.js
> Warning Failed to make bytecode node16-x64 for file /snapshot/restjs/node_modules/node-fetch/src/utils/is-redirect.js
> Warning Failed to make bytecode node16-x64 for file /snapshot/restjs/node_modules/node-fetch/src/utils/is.js
> Warning Failed to make bytecode node16-x64 for file /snapshot/restjs/node_modules/node-fetch/src/utils/referrer.js
> Warning Failed to make bytecode node16-x64 for file /snapshot/restjs/node_modules/data-uri-to-buffer/dist/index.js
> Warning Failed to make bytecode node16-x64 for file /snapshot/restjs/node_modules/fetch-blob/index.js
> Warning Failed to make bytecode node16-x64 for file /snapshot/restjs/node_modules/formdata-polyfill/esm.min.js
> Warning Failed to make bytecode node16-x64 for file /snapshot/restjs/node_modules/fetch-blob/from.js
> Warning Failed to make bytecode node16-x64 for file /snapshot/restjs/node_modules/fetch-blob/file.js

Having dependencies on various major release versions seems like unnecessary complexity to me. How many people are using pkg anyways? Seems like I'm going back to fiddling with plain old rest requests silently hoping the pkg guys are adding ESM support :(