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

SuperJson Jest SWC run issue #184

Open
luchillo17 opened this issue Jan 31, 2024 · 13 comments
Open

SuperJson Jest SWC run issue #184

luchillo17 opened this issue Jan 31, 2024 · 13 comments

Comments

@luchillo17
Copy link

luchillo17 commented Jan 31, 2024

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
Copy link

andrea-benato-deltatre commented Jan 31, 2024

same using vitest
apparently a dependencies is using CommonJS
/node_modules/.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
Copy link
Member

phryneas commented Feb 1, 2024

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
Copy link
Author

luchillo17 commented Feb 2, 2024

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
Copy link

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
Copy link

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
Copy link
Member

phryneas commented Feb 2, 2024

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
Copy link

andrea-benato-deltatre commented Feb 2, 2024

@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
Copy link
Author

luchillo17 commented Feb 2, 2024

@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

// 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',
};
// 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

// 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',
};
// 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
Copy link
Author

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
Copy link
Member

phryneas commented Feb 8, 2024

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
Copy link
Author

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
Copy link
Author

luchillo17 commented Feb 13, 2024

@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
Copy link
Author

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...

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

No branches or pull requests

3 participants