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

Potential issues with ArcGIS REST JS v4 and Jest #1008

Closed
patrickarlt opened this issue Jul 28, 2022 · 2 comments
Closed

Potential issues with ArcGIS REST JS v4 and Jest #1008

patrickarlt opened this issue Jul 28, 2022 · 2 comments
Assignees

Comments

@patrickarlt
Copy link
Contributor

patrickarlt commented Jul 28, 2022

TL:DR; if you want to use ArcGIS REST JS v4 in Jest you need to enable the experimental support for ESM in Jest. If you also using ts-jest you need to enable support for ESM in ts-jest as well.

This is discussed more thoroughly in #993. I'm going to break out the discussion into a new issue. There are currently some issues when using ArcGIS REST JS v4 with Jest:

  1. You need to use >jest@28.0.0 to get support for conditional imports. This allows Jest to properly load the CJS or ESM modules.
  2. Jest will now load the CJS version of @esri/arcgis-rest-fetch. This runs into Segmentation fault with import() instead of calling importModuleDynamically nodejs/node#35889.
  3. To avoid the sef faul on dynamic imports you need to enable the experimental support for ESM in Jest. If you are also using TypeScript + Jest via ts-jest you need to enable support for ESM in ts-jest as well.

Doing all the config steps for ESM in Jest/ts-jest doesn't seem entirely necessary. For example https://github.com/sandromartis/arcgis-rest-js-segfault/pull/1 I was able to not include the recommended transform config and it still worked. I was also able to leave module: commonjs in the package.json. Your mileage may vary on these things.


It would be great if Jest worked OOTB either by supporting ESM OOTB or if the seg faul bug was fixed. We can either:

  1. Hope the segmentation fault bug is resolved in Node JS 20. this should resolve the issue but would require users to upgrade to Jest 28+ and Node 20+ which would be April 2023 at the earliest.

  2. Split the @esri/arcgis-rest-fetch package into 3 packages.

    1. A @esri/arcgis-rest-fetch package which loads:
    2. @esri/arcgis-rest-fetch-esm which uses node-fetch@3 for ESM environments
    3. @esri/arcgis-rest-fetch-cjs which uses node-fetch@2 for Common JS environments

    This avoids the dynamic import which avoids the segmentation fault error.

  3. Switch to Undici and work on ArcGIS REST JS 5.0.0. This would require Node 16+ (the current LTS) and use Undici which is a CommonJS module in Node 16 and optionally use the built in fetch at Node 18+. Since you can import CJS into
    modules into ESM this avoids a dynamic import.


I don't really like any of these, however with lots of teams using Jest inside Esri this will hinder adoption of v4.

  1. Seems like we are just waiting for a fix for FAR to long. If this doesn't get released in April 2023 this might persist until 2025 and we just have to implement other options anyway.
  2. This seems like a maintenance nightmare and reverting to node-fetch@2 might introduce who-knows-what security and other bugs.
  3. This seems premature given that we just released v4 and wanted to wait on v5 so we could incorporate some other work. Rushing v5 to fix what might just be a shoddy implementation in a popular test framework doesn't feel right.

I think my final call would be to see if this gets fixed in Node 20. If it does great. If it doesn't start work on ArcGIS REST JS 5 and switch to Undici. The other option is to just make your Jest tests use ES modules which does work today.

@patrickarlt
Copy link
Contributor Author

patrickarlt commented Jul 29, 2022

This might not be as bad as I originally feared. I have a potential solution that worked for @sandromartis (@mpayson is still testing). This config for Jest appears to work https://github.com/sandromartis/arcgis-rest-js-segfault/pull/1/files with the TypeScript config.

With the recent success of these configs I think we are OK to just tell people to enable the ESM support and hope that the Node JS team stabilizes it soon.

@patrickarlt
Copy link
Contributor Author

Closing this but leaving it pinned. The solution in https://github.com/sandromartis/arcgis-rest-js-segfault/pull/1/files seems to work.

@patrickarlt patrickarlt unpinned this issue Dec 19, 2023
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

No branches or pull requests

4 participants