apollographql / apollo-client-nextjs

Apollo Client support for the Next.js App Router
https://www.npmjs.com/package/@apollo/experimental-nextjs-app-support
MIT License
358 stars 25 forks source link

SuperJson Jest SWC run issue #184

Open luchillo17 opened 3 months ago

luchillo17 commented 3 months ago

Issue

I tried recently in a new project to use this library again, not sure why but this time I can't run the tests, from the error message I can only infer it's the SuperJson library that has a non-commonjs dist index.js, have yet to find how to configure stuff so it includes that specific library in the compilation of TS.

To be clear I do have other components & tests with imports, it's this one specifically that presents this error. Screenshot 2024-01-31 022901

andrea-benato-deltatre commented 3 months ago

same using vitest apparently a dependencies is using CommonJS /nodemodules/.pnpm/@apollo+experimental-nextjs-app-support@0.7.0@apollo+client@3.9.0_next@14.1.0_react@18.2.0/node_modules/@apollo/experimental-nextjs-app-support/dist/ssr/dataTransport.js to a dynamic import() which is available in all CommonJS modules. i

phryneas commented 3 months ago

You might try to use a package.json overrides field to specify if you want to use superjson version 1 or 2 - I believe v1 is commonJs, while v2 is ESM.

luchillo17 commented 3 months ago

I deleted a comment about a different bug, the overrides do work though I'm using pnpm so I had to do pnpm.overrides in the package.json, still it is an obscure issue, might be a good idea to mention in the README that this kind of thing can happen and is fixable like this.

andrea-benato-deltatre commented 3 months ago

tried it but at least with vitest the problem is still there. I've also tried to install in the project the correct superjson version but same error:

Error: require() of ES Module node_modules/.pnpm/superjson@2.2.1/node_modules/superjson/dist/index.js from /home/aabeborn/dev/ioc/ioc.web.results.graphql.connector/node_modules/.pnpm/@apollo+experimental-nextjs-app-support@0.8.0_@apollo+client@3.9.0_next@14.1.0_react@18.2.0/node_modules/@apollo/experimental-nextjs-app-support/dist/ssr/dataTransport.js not supported.
Instead change the require of index.js in /node_modules/.pnpm/@apollo+experimental-nextjs-app-support@0.8.0_@apollo+client@3.9.0_next@14.1.0_react@18.2.0/node_modules/@apollo/experimental-nextjs-app-support/dist/ssr/dataTransport.js to a dynamic import() which is available in all CommonJS modules.
andrea-benato-deltatre commented 3 months ago

issue with vitest is that dataTransport.js for example is compiled in this way:

const superjson_1 = __importDefault(require("superjson"));
const ApolloRehydrateSymbols_1 = require("./ApolloRehydrateSymbols");
const lateInitializingQueue_1 = require("./lateInitializingQueue");

vitest doesn't work with require so problem is not superjson (maybe also it, but not the only one) but how it is bundled the package

phryneas commented 3 months ago

It's a bit of a weird situation - we're compiling the package in a way that works well for Next.js, because that's our "target consumer".

If your test environment now errors, I'm inclined to say "your test setup needs to be in line with your Next.js bundling setup, because otherwise your tests probably test completely different code that you actually bundle and run in production".

But at the same time, I also feel your pain.

In the short-to-medium term (talking days to weeks probably), I'm going to abstract away the core of this package to also work with other frameworks like Redwood, and at that time I'll have to revisit the bundling and make sure it works in more environments.

In the very short term, I'm sorry I don't have a better solution.
With jest, you can probably adjust your babel setup, but with vitest I'm not sure I have any immediate advice, sorry.

One last thing:

vitest doesn't work with require

I'm a bit confused how vitest can even use React then, which heavily uses require and doesn't ship ESM? 🤔

andrea-benato-deltatre commented 3 months ago

@phryneas thank you for the reply. I don't now exactly I'm kind of new to vitest. Maybe it's related to the fact that react has a specific plugin for it? It could make sense at least in my mind. Also idk, maybe they don't have dynamic imports inside the bundle. Right now I'm simply skipping tests so it's not a big deal

luchillo17 commented 3 months ago

@phryneas It is indeed a weir case, to clarify my situation, I have an Nrwl Nx Monorepo with Next.js configured, but I don't have this in the main app, I put it into a NextJS Library, which I think doesn't repro the NextJS build setup 1 to 1, for example, the jest configs and the TS configs already differ, the biggest diff I can spot is the transform, Next uses babel-jest while the library is using @swc/jest.

NextJS files

```ts // Next jest.confi.ts /* eslint-disable */ export default { displayName: 'graph-budget', preset: '../../jest.preset.js', transform: { '^(?!.*\\.(js|jsx|ts|tsx|css|json)$)': '@nx/react/plugins/jest', '^.+\\.[tj]sx?$': ['babel-jest', { presets: ['@nx/next/babel'] }], }, moduleFileExtensions: ['ts', 'tsx', 'js', 'jsx'], coverageDirectory: '../../coverage/apps/graph-budget', }; ``` ```json // Next tsconfig.spec.json { "extends": "./tsconfig.json", "compilerOptions": { "outDir": "../../dist/out-tsc", "module": "commonjs", "types": ["jest", "node"], "jsx": "react" }, "include": [ "jest.config.ts", "src/**/*.test.ts", "src/**/*.spec.ts", "src/**/*.test.tsx", "src/**/*.spec.tsx", "src/**/*.test.js", "src/**/*.spec.js", "src/**/*.test.jsx", "src/**/*.spec.jsx", "src/**/*.d.ts" ] } ```

NextJS Library files

```ts // Library jest.confi.ts /* eslint-disable */ export default { displayName: 'neo4j-apollo', preset: '../../jest.preset.js', transform: { '^.+\\.[tj]sx?$': [ '@swc/jest', { jsc: { parser: { syntax: 'typescript', tsx: true }, transform: { react: { runtime: 'automatic' } }, }, }, ], }, moduleFileExtensions: ['ts', 'tsx', 'js', 'jsx'], coverageDirectory: '../../coverage/libs/neo4j-apollo', }; ``` ```json // Library tsconfig.spec.json { "extends": "./tsconfig.json", "compilerOptions": { "outDir": "../../dist/out-tsc", "module": "commonjs", "types": ["jest", "node"] }, "include": [ "jest.config.ts", "src/**/*.test.ts", "src/**/*.spec.ts", "src/**/*.test.tsx", "src/**/*.spec.tsx", "src/**/*.test.js", "src/**/*.spec.js", "src/**/*.test.jsx", "src/**/*.spec.jsx", "src/**/*.d.ts" ] } ```

luchillo17 commented 3 months ago

You might try to use a package.json overrides field to specify if you want to use superjson version 1 or 2 - I believe v1 is commonJs, while v2 is ESM.

To avoid confusion for people with my latest comment, this is the latest workaround, not quite a definitive solution though.

phryneas commented 3 months ago

To keep you updated, I'm reworking the bundling (among other things) over in #189.

There will be CommonJS and ESM for this package, depending on what your environment needs.

There is just one problem arising: Packages like this should make use of exports conditions to determine which bundle/code will be imported.

So far, we didn't do that, but we had checks like typeof window === "undefined" to determine if we are in the browser or in nodejs.

So, assuming you were using something like JSDOM until now, which "fakes" the global window object, your tests were always running the "browser version" of the hooks. With this new approach, I believe your tests will be running the "SSR version" of the hooks, unless you manually run your tests with --conditions browser and without --conditions node (your test runner might add that one automatically though?).

I'm not 100% sure if that will work out nicely. Could you all please try out the PR release over in that other issue and give feedback if you get that running like you'd expect it to work over there?

luchillo17 commented 3 months ago

Wow, those are a lot of file changes, seems like you even moved stuff out of the global window, I'm not confident in my React & Next knowledge to understand all that, but I'll try looking into it on the weekend, as well as trying it out in my project.

luchillo17 commented 2 months ago

@phryneas I installed the PR commit, removed the npm overrides (pnpm.overrides in my case), and ran my app, works like a charm, not sure what other cases we can test, but it looks to me like it fixes this issue with no problem.

Nevermind, forgot this issue was with tests, with the PR installed I'm still having this issue, though now there's no mention of SuperJson:

image

luchillo17 commented 2 months ago

On the other hand, I went and installed the latest 0.8.0, removed the overrides and the tests work ok now (though I did have to wrap my components in <MockedProvider mocks={mocks}> so I'm starting to wonder if this was part of the issue before.

Maybe we can close this issue given the latest version works ok without workarounds...