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

fix #5 due to no esmodule #6

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

parisholley
Copy link

No description provided.

@james-yeoman
Copy link

james-yeoman commented Oct 17, 2023

I've tried your fork, but you only have the fix for ESM. I'm using esbuild to bundle for serverless, which ends up being a CJS environment (node 18).

So I'm trying to bundle ESM to CJS, while importing a module that has a CJS file, which has a Zod import, which as we know, breaks instanceof checks.
image

Maybe it's worth adding an exports map? That might cause ESBuild to use the ESM version when importing, and compile it to CJS so that it shares the CJS Zod import?

Something like

"exports": {
	"types": {
		"import": "dist/esm/index.d.ts",
		"require": "dist/cjs/index.d.ts"
	}
	"import": "dist/esm/index.js",
	"require": "dist/cjs/index.js"
}

Alternatively, maybe Zod needs to be externalised? That would also solve the problem, because then ESBuild would be able to define Zod for this package, rather than this package having it's own Zod import.

I had a look at the build artifacts and Zod is already externalised.

So the problem stems from the fact that when targeting CJS (which is necessary when using the Serverless framework for deploying to AWS Lambda), the CJS version is being resolved.

So the only way I can see this problem being resolved is with the Exports map. The JS ecosystem has struck again ☹️

@james-yeoman
Copy link

Additionally, the ESBuild docs state the following

When the platform is set to node

The main fields setting is set to main,module. This means tree shaking will likely not happen for packages that provide both module and main since tree shaking works with ECMAScript modules but not with CommonJS

Unfortunately some packages incorrectly treat module as meaning "browser code" instead of "ECMAScript module code" so this default behavior is required for compatibility. You can manually configure the main fields setting to module,main if you want to enable tree shaking and know it is safe to do so.

Unfortunately for me, I'm unable to set the main fields setting to module, main. When I do so, some other package breaks, and my GraphQL Schema becomes invalid. Still not sure what's breaking, but it's not as simple as it seems

"types": "dist/index.d.ts",
"main": "dist/cjs/index.js",
"module": "dist/esm/index.js",
"types": "dist/cjs/index.d.ts",
Copy link

@james-yeoman james-yeoman Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used yarn patch to try the exports map, and can confirm that it solves the ESBuild problem.

Suggested change
"types": "dist/cjs/index.d.ts",
"types": "dist/cjs/index.d.ts",
"exports": {
"types": {
"import": "./dist/esm/index.d.ts",
"default": "./dist/cjs/index.d.ts"
},
"import": "./dist/esm/index.js",
"default": "./dist/cjs/index.js"
},

@james-yeoman
Copy link

@parisholley I don't suppose you've got notifications disabled for this PR, have you? I can confirm that the exports map works perfectly

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

Successfully merging this pull request may close these issues.

2 participants