ourzora / zora-protocol

Monorepo for Zora Protocol (contracts & sdks)
MIT License
138 stars 87 forks source link

Recommendation: Utilize exports field in package.json for better module resolution and compatibility #403

Closed Quazia closed 8 months ago

Quazia commented 8 months ago

Right now it can be difficult integrating your SDK due to the lack of an exports field in your package.json. This omission leads to potential resolution and compatibility problems, especially in projects that use a mix of ES Modules and CommonJS.

It would be great if you could build CJS and ESM compatibly exports into your dist folder to make the SDK easier to work with.

oveddan commented 8 months ago

thanks for reporting, will look into it. Could you add an example of issues you run into with projects that use a mix of ES Modules and CommonJS?

Quazia commented 8 months ago

@oveddan Here's where we're consuming your SDK

This import pattern is icky and I don't like it. If we don't do this we get an error shaped like this:

[Node] Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/arthurlunn/Documents/GitHub/rh/node_modules/@zoralabs/protocol-sdk/dist/index.js from /Users/arthurlunn/Documents/GitHub/rh/node_modules/@rabbitholegg/questdk-plugin-zora/dist/cjs/Zora.js not supported.

Here's an example of how our build process handles this

You can also see the export pattern in this same file

Happy to help with any further insights into how we'd prefer to consume your SDK 😊

Quazia commented 8 months ago

Just want to flag the fix we're using here did fix the build time issues but we're still running into some runtime issues so a fix for this would be super helpful.

iainnash commented 8 months ago

Looking.

Quazia commented 8 months ago

Looking.

Let me know if y'all plan on exporting additional builds and what the timeline would be cause alternatively we can just tweak our build. Semi time sensitive, need this resolved one way or another end of next week but like I said we can get around this on our end if needed just more work.

oveddan commented 8 months ago

Hi @Quazia I'm looking more at the issue;

Screenshot 2024-02-28 at 2 22 28 PM

which would need to be fixed in your local typescript config if you wanted to continue to use dynamic import.

If you move the import to the top, it is able to go to definition and load the type successfully:

Screenshot 2024-02-28 at 2 24 50 PM
oveddan commented 8 months ago

whats the actual command youre running when you get the error: [Node] Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/arthurlunn/Documents/GitHub/rh/node_modules/@zoralabs/protocol-sdk/dist/index.js from /Users/arthurlunn/Documents/GitHub/rh/node_modules/@rabbitholegg/questdk-plugin-zora/dist/cjs/Zora.js not supported."

could you print out the full output?

oveddan commented 8 months ago

disregard my first comment - i'm looking a bit more into the exports field

oveddan commented 8 months ago

I was actually able to get things to compile without using dynamic import, by moving the import statement to the top of the file like:

import { type MintIntentParams } from '@rabbitholegg/questdk-plugin-utils'
import { getMintCosts, MintAPIClient } from '@zoralabs/protocol-sdk';
pnpm i
pnpm build

output:


> @rabbitholegg/questdk-plugin-zora@1.0.0-alpha.18 build /Users/danovedzora/source/examples/questdk-plugins/packages/zora
> pnpm run clean && pnpm run build:cjs && pnpm run build:esm && pnpm run build:types

> @rabbitholegg/questdk-plugin-zora@1.0.0-alpha.18 clean /Users/danovedzora/source/examples/questdk-plugins/packages/zora
> rimraf dist

> @rabbitholegg/questdk-plugin-zora@1.0.0-alpha.18 build:cjs /Users/danovedzora/source/examples/questdk-plugins/packages/zora
> tsc --project tsconfig.build.json --module commonjs --outDir ./dist/cjs --removeComments --verbatimModuleSyntax false && echo > ./dist/cjs/package.json '{"type":"commonjs"}'

> @rabbitholegg/questdk-plugin-zora@1.0.0-alpha.18 build:esm /Users/danovedzora/source/examples/questdk-plugins/packages/zora
> tsc --project tsconfig.build.json --module es2015 --outDir ./dist/esm && echo > ./dist/esm/package.json '{"type":"module","sideEffects":false}'

> @rabbitholegg/questdk-plugin-zora@1.0.0-alpha.18 build:types /Users/danovedzora/source/examples/questdk-plugins/packages/zora
> tsc --project tsconfig.build.json --module esnext --declarationDir ./dist/types --emitDeclarationOnly --declaration --declarationMap
Quazia commented 8 months ago

That builds but the code in the dist folder is giving the error I shared when you try and consume it upstream.

Let me try and get you a more complete error from the package that consumes this dependency.

jonathandiep commented 8 months ago

@oveddan Originally we did use the import without making them dynamic, but ran into issues at build time

Here is the error that we get at runtime when we try to use our SDK, which uses the Zora SDK

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/jon/rh-repo/node_modules/.pnpm/@zoralabs+protocol-sdk@0.5.3_@types+node@16.18.82_typescript@5.3.3_viem@1.21.4_zod@3.22.4/node_modules/@zoralabs/protocol-sdk/dist/index.js from /Users/jon/rh-repo/node_modules/.pnpm/@rabbitholegg+questdk-plugin-zora@1.0.0-alpha.18_@types+node@16.18.82_typescript@5.3.3_viem@1.21.4_zod@3.22.4/node_modules/@rabbitholegg/questdk-plugin-zora/dist/cjs/Zora.js not supported.
Instead change the require of index.js in /Users/jon/rh-repo/node_modules/.pnpm/@rabbitholegg+questdk-plugin-zora@1.0.0-alpha.18_@types+node@16.18.82_typescript@5.3.3_viem@1.21.4_zod@3.22.4/node_modules/@rabbitholegg/questdk-plugin-zora/dist/cjs/Zora.js to a dynamic import() which is available in all CommonJS modules.
    at Hook.Module.require (/Users/jon/rh-repo/node_modules/.pnpm/dd-trace@4.27.0/node_modules/dd-trace/packages/dd-trace/src/ritm.js:85:33)
    at /Users/jon/rh-repo/node_modules/.pnpm/@rabbitholegg+questdk-plugin-zora@1.0.0-alpha.18_@types+node@16.18.82_typescript@5.3.3_viem@1.21.4_zod@3.22.4/node_modules/@rabbitholegg/questdk-plugin-zora/dist/cjs/Zora.js:113:93
    at async getProjectFees (/Users/jon/rh-repo/node_modules/.pnpm/@rabbitholegg+questdk-plugin-zora@1.0.0-alpha.18_@types+node@16.18.82_typescript@5.3.3_viem@1.21.4_zod@3.22.4/node_modules/@rabbitholegg/questdk-plugin-zora/dist/cjs/Zora.js:113:45) {
  code: 'ERR_REQUIRE_ESM'
}

So I think what we need is that the Zora SDK can eventually be compiled in CommonJS run time

oveddan commented 8 months ago

@jonathandiep @Quazia thx for clarifying. weve published a tag with this suggested fix at @zoralabs/protocol-sdk@0.5.4-exports.0 - could you please try installing that version and seeing if that fixes the issue?

here's the updated package.json:

{
  "name": "@zoralabs/protocol-sdk",
  "version": "0.5.4-exports.0",
  "repository": "https://github.com/ourzora/zora-protocol",
  "license": "MIT",
  "main": "./dist/index.cjs",
  "module": "./dist/index.js",
  "types": "./dist/index.d.ts",
  "exports": {
    ".": {
      "types": "./dist/index.d.ts",
      "import": "./dist/index.js",
      "default": "./dist/index.cjs"
    }
  },
  "type": "module",
  "scripts": {
    "build": "tsup",
    "prepack": "yarn build",
    "test:js": "vitest src",
    "test:integration": "vitest test-integration",
    "generate-types": "echo 'npx is used here because this is a rare operation' && npx openapi-typescript https://api.zora.co/premint/openapi.json -o src/generated/premint-api-types.ts && yarn openapi-typescript https://api.zora.co/discover/openapi.json -o src/generated/discover-api-types.ts",
    "prettier": "prettier --write 'src/**/*.ts' 'test-integration/**/*.ts'",
    "lint": "prettier --check 'src/**/*.ts' 'test-integration/**/*.ts'"
  },
  "dependencies": {
    "@zoralabs/protocol-deployments": "*",
    "abitype": "^0.10.3",
    "vite": "4.5.0"
  },
  "peerDependencies": {
    "viem": "^1.19.15"
  },
  "devDependencies": {
    "@lavamoat/preinstall-always-fail": "2.0.0",
    "zoralabs-tsconfig": "*",
    "typescript": "^5.2.2",
    "vite": "^4.5.0",
    "vitest": "^0.34.6"
  }
}
iainnash commented 8 months ago

We've added the exports field in a prerelease version https://www.npmjs.com/package/@zoralabs/protocol-sdk/v/0.5.4-exports.0?activeTab=versions

Quazia commented 8 months ago

@iainnash @oveddan Amazing thank you! Will get this tested this afternoon and report back. Really appreciate the promptness.

jonathandiep commented 8 months ago

@iainnash @oveddan Seems like upgrading to 0.5.4-exports.0 produces a different kind of error

Here's what we're seeing when we upgrade and try to import from @zoralabs/protocol-sdk

Cannot find module '@zoralabs/protocol-sdk' or its corresponding type declarations

Can reproduce in our SDK repo here: https://github.com/rabbitholegg/questdk-plugins/tree/jon/zora-protocol-sdk-export-error

But lmk if there's any issue or anything I can report to help debug

jonathandiep commented 8 months ago

One difference that I am seeing between 0.5.4 and 0.5.4-exports.0 is that 0.5.4 will build and create a dist folder

Doesn't seem like 0.5.4-exports.0 does that, which probably explains why our SDK can't find the @zoralabs/protocol-sdk module

oveddan commented 8 months ago

yeah just noticed this too. adding the exports is going to require us to define all the files to include in the package in the files setting which isn't ideal. looking into a fix

oveddan commented 8 months ago

Hey @jonathandiep we think we figured out the issue, related to this - we've added a .npmignore and made sure to not exclude the dist - can you try again with the tag @zoralabs/protocol-sdk@0.5.4-exports.3

jonathandiep commented 8 months ago

@oveddan Seems like we're getting closer as the issue doesn't happen on the @zoralabs/protocol-sdk package, but now happens as a part of the @zoralabs/protocol-deployments that it uses lol

Here's the error that we're running into:

/Users/jon/questdk-plugins/node_modules/.pnpm/@zoralabs+protocol-sdk@0.5.4-exports.3_@types+node@20.4.5_ts-node@10.9.1_typescript@5.3.2_viem@2.7.9/node_modules/@zoralabs/protocol-sdk/dist/index.cjs:70
var import_protocol_deployments2 = require("@zoralabs/protocol-deployments");
                                   ^
Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/jon/questdk-plugins/node_modules/.pnpm/@zoralabs+protocol-deployments@0.1.0_@types+node@20.4.5_ts-node@10.9.1/node_modules/@zoralabs/protocol-deployments/dist/index.js from /Users/jon/questdk-plugins/node_modules/.pnpm/@zoralabs+protocol-sdk@0.5.4-exports.3_@types+node@20.4.5_ts-node@10.9.1_typescript@5.3.2_viem@2.7.9/node_modules/@zoralabs/protocol-sdk/dist/index.cjs not supported.
Instead change the require of index.js in /Users/jon/questdk-plugins/node_modules/.pnpm/@zoralabs+protocol-sdk@0.5.4-exports.3_@types+node@20.4.5_ts-node@10.9.1_typescript@5.3.2_viem@2.7.9/node_modules/@zoralabs/protocol-sdk/dist/index.cjs to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/Users/jon/questdk-plugins/node_modules/.pnpm/@zoralabs+protocol-sdk@0.5.4-exports.3_@types+node@20.4.5_ts-node@10.9.1_typescript@5.3.2_viem@2.7.9/node_modules/@zoralabs/protocol-sdk/dist/index.cjs:70:36)
    at Object.<anonymous> (/Users/jon/questdk-plugins/packages/zora/dist/cjs/Zora.js:6:24)
    at Object.<anonymous> (/Users/jon/questdk-plugins/packages/zora/dist/cjs/index.js:5:19)
    at Object.<anonymous> (/Users/jon/questdk-plugins/packages/registry/dist/cjs/index.js:36:31)

It seems like we can apply the same fixes to the @zoralabs/protocol-deployments package?

oveddan commented 8 months ago

that makes sense @jonathandiep I've updated the depended on packages, can you try now with tag: @zoralabs/protocol-sdk@0.5.6-exports.0

Quazia commented 8 months ago

Looks like it's all working now! Thanks for the help @oveddan and @iainnash

oveddan commented 8 months ago

Great to hear