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 vitest incompatibility #3674

Closed
wants to merge 2 commits into from
Closed

fix vitest incompatibility #3674

wants to merge 2 commits into from

Conversation

capaj
Copy link

@capaj capaj commented Jul 13, 2022

ATM vitest/vite cannot load graphql correctly. It always ends up loading both CJS and ESM source in my projects.
I have reported this to vite here: vitejs/vite#7879
but I think a simple change here could resolve the problem. I think using "module" entry point is obsolete/deprecated with node.js.

I have tested this change with latest vitests and it seems to work correctly with this change. I will be using patch-package with graphql 16 for now, but it would be nice if this could land.

Also why was private: true set here?

ATM vitest/vite cannot load graphql correctly.
I have reported this to vite here: vitejs/vite#7879

I have tested this change with latest vitests and it seems to work correctly with this change. I will be using patch-package with graphql 16 for now, but it would be nice if this could land.

Also why was `private: true` set here?
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 13, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: capaj / name: Jiri Spac (4412cd8)

@n1ru4l
Copy link
Contributor

n1ru4l commented Jul 13, 2022

@capaj Even on Node.js if you load graphql in an ESM context it will load the common.js version because the published package does not have an exports map 🤷‍♂️ (See this code-sandbox: https://codesandbox.io/s/graphql-module-resolution-on-node-js-issue-xk5w8t?file=/index.js:0-398). "module": "index.mjs", is completely ignored by Node.js 😁.

This is the published package.json: https://unpkg.com/graphql@16.5.0/package.json

Also why was private: true set here?

Because the npm package is built in a custom way:
https://github.com/graphql/graphql-js/blob/main/resources/build-npm.ts


A list of related issues/prs:

@capaj
Copy link
Author

capaj commented Jul 13, 2022

I'll revert that change with private then, thanks @n1ru4l

@n1ru4l
Copy link
Contributor

n1ru4l commented Jul 13, 2022

I also think your PR on v16 will not be accepted because adding an exports map breaks all the people that do "deep imports" today, thus it is considered a breaking change. 😁

Another fun fact is that graphql-js does not work on CodeSandbox frontend templates since v15 was released.

@capaj
Copy link
Author

capaj commented Jul 13, 2022

Sounds about right. It would be best to just revert back to pure CJS without introducing and exports map.

@capaj
Copy link
Author

capaj commented Jul 13, 2022

I should have checked your links before responding :D
I will just use https://npm.im/graphql-esm from #3361

seems like it is an official package-might be worth adding to the docs.
Let's close this one, hopefully @PabloSzx has got it

@capaj capaj closed this Jul 13, 2022
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