contentstack / contentstack-javascript

Javascript SDK for Contentstack's Content Delivery API (Nodejs and Browser)
MIT License
25 stars 21 forks source link

Contentstack package depends on isomorphic-fetch, doesn't use it #99

Open cefn opened 1 year ago

cefn commented 1 year ago

In our test suite (running in node) it seems depending on contentstack creates a broken package dependency situation.

At https://github.com/contentstack/contentstack-javascript/blob/290cda73b1a9df0f697611cee28cc1992f2929b8/src/runtime/node/http.js#L2 the npm contentstack package imports node-fetch. However, it doesn't declare node-fetch among its dependencies.

The "contentstack" package correctly declares isomorphic-fetch among its dependencies. The isomorphic-fetch project allows packages to be defined which are neither node- or browser- specific, with the isomorphic-fetch package resolving to the appropriate packages at runtime.

However, because contentstack incorrectly imports node-fetch instead of isomorphic-fetch this forces projects to explicitly install node-fetch to prevent runtime errors when testing in node. This defeats the purpose of isomorphism, meaning that packages which are ONLY intended for client-side deployment end up needing a node-fetch dependency which makes no sense.

cefn commented 1 year ago

For reference this was detectable (and problematic) since node-fetch was resolved (again implicitly) to a version which was installed by another package in our monorepo. Unlike the node-fetch v2 dependency declared by isomorphic-fetch, the node-fetch v3 dependency which was then resolved (implicitly because it was undeclared by contentstack) turned out to be an ESM package, leading to the following error...

 RUN  v0.32.2 /Users/superman/workspace/scratch/contentstack-schema-iac/packages/stack-renderer

 ❯ test/transform.test.ts (0)

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Suites 1 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

 FAIL  test/transform.test.ts [ test/transform.test.ts ]
Error: require() of ES Module /Users/superman/workspace/scratch/contentstack-schema-iac/node_modules/node-fetch/src/index.js from /Users/superman/workspace/scratch/contentstack-schema-iac/node_modules/contentstack/dist/node/contentstack.js not supported.
Instead change the require of index.js in /Users/superman/workspace/scratch/contentstack-schema-iac/node_modules/contentstack/dist/node/contentstack.js to a dynamic import() which is available in all CommonJS modules.
 ❯ 809 ../../node_modules/contentstack/dist/node/contentstack.js:1:37275
 ❯ r ../../node_modules/contentstack/dist/node/contentstack.js:1:37397
 ❯ 289 ../../node_modules/contentstack/dist/node/contentstack.js:1:36890
 ❯ r ../../node_modules/contentstack/dist/node/contentstack.js:1:37397
 ❯ 777 ../../node_modules/contentstack/dist/node/contentstack.js:1:4449
 ❯ r ../../node_modules/contentstack/dist/node/contentstack.js:1:37397
 ❯ 540 ../../node_modules/contentstack/dist/node/contentstack.js:1:11074
 ❯ r ../../node_modules/contentstack/dist/node/contentstack.js:1:37397
 ❯ 533 ../../node_modules/contentstack/dist/node/contentstack.js:1:29017
 ❯ r ../../node_modules/contentstack/dist/node/contentstack.js:1:37397
 ❯ 32 ../../node_modules/contentstack/dist/node/contentstack.js:1:2627
 ❯ r ../../node_modules/contentstack/dist/node/contentstack.js:1:37397
 ❯ ../../node_modules/contentstack/dist/node/contentstack.js:1:37423
 ❯ Object.<anonymous> ../../node_modules/contentstack/dist/node/contentstack.js:1:37446
 ❯ async /Users/superman/workspace/scratch/contentstack-schema-iac/packages/stack-iac/src/deliver.ts:3:31
 ❯ async /Users/superman/workspace/scratch/contentstack-schema-iac/packages/stack-iac/src/index.ts:3:31
 ❯ async /Users/superman/workspace/scratch/contentstack-schema-iac/packages/stack-renderer/src/transform.ts:2:31
 ❯ async /Users/superman/workspace/scratch/contentstack-schema-iac/packages/stack-renderer/test/transform.test.ts:2:31

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: {
  "code": "ERR_REQUIRE_ESM",
}
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯

 Test Files  1 failed (1)
      Tests  no tests
   Start at  14:36:06
   Duration  323ms (transform 92ms, setup 0ms, collect 0ms, tests 0ms, environment 0ms, prepare 77ms)
cefn commented 1 year ago

For reference a proven version of node-fetch that can be installed by a host project to make contentstack happy is "node-fetch": "^2.6.12". However, this is an incorrect resolution for client-side packages which intend to use contentstack isomorphically (and therefore using only browser fetch in production).

ishaileshmishra commented 1 year ago

Thank you for bringing this to our attention @cefn. We greatly appreciate your effort in reporting the issue. Our team is currently investigating the matter and looking into the details provided.

Mcbeer commented 8 months ago

Any updates on this issue?

cefn commented 1 month ago

Please make this package stable.