Open kzunino opened 3 weeks ago
Thank you for reporting this!
You're right, this is a check that we added newly, and it's probably a bit too strict.
I'm changing it over in #284 so that the SSR build of ApolloNextAppProvider
will still error, but you can use the browser build.
Jest should automatically pick up the browser build, but I believe Vite will set you up with the SSR build per default.
You can see this vite.config.js
to see what you need to adjust to make vite pick up the browser build instead.
Keep in mind that you also call the cleanup function after each test:
import { resetNextSSRApolloSingletons } from "@apollo/experimental-nextjs-app-support/ssr";
afterEach(resetNextSSRApolloSingletons);
Could you please try the PR release from that PR and report back if that does the trick for you?
npm i @apollo/experimental-nextjs-app-support@0.0.0-commit-ef5e30b
Good morning @phryneas. Thanks for your speedy response!
I still have issues after updating the version to @0.0.0-commit-ef5e30b
.
I previously added afterEach(resetNextSSRApolloSingletons);
to my setupTests to run after each suite. So no issues there. Thanks for the callout!
I did not have two of those settings in my vitetest.config
file mentioned in the links (server.deps.inline: true
or resolve.conditions['browser']
). I'm still encountering errors. However, these errors seem related to Vitest and MSW because because my setupTests.ts
file starts a server to listen for API calls for MSW and by changing the conditions to browser
, I don't think Vitest likes the setupServer import since it runs in node: import { setupServer } from 'msw/node';
.
Here's an article that kinda explains our implementation (if that is helpful). Note: We don't have the client side serviceWorker
setup because we had issues with AppRouter and the way it monkey patches fetch requests and caches data. Supposedly there is movement to address this issue but I haven't kept up with it lately. We do have MSW configured for testing. You can see this explanation under MSW and React Testing in the article, which uses the node-based setupServer
.
The gist of the implementation is here in setupTests. The issue I think is that worker is actually coming from a node package.
// ./setupTests
import { expect, afterEach, beforeAll, afterAll } from 'vitest';
import * as matchers from "@testing-library/jest-dom/matchers";
import { worker } from './msw/workers';
expect.extend(matchers);
// Start worker before all tests
beforeAll(() => { worker.listen() })
// Close worker after all tests
afterAll(() => {worker.close()})
// Reset handlers after each test `important for test isolation`
afterEach(() => {worker.resetHandlers()})
I'll try to adjust my implementation to see if I can get the serviceWorker to work instead for tests. I'll have to check back later for that
Thanks!
------------ Context -----------
Here's context of what I have tried — if I just update the package version without modifying Vitest I see errors like the one below. I think this eludes to needed the browser build since next navigation imports client side hooks.
Error: Cannot find module '/.../node_modules/next/navigation' imported from /.../node_modules/@apollo/experimental-nextjs-app-support/dist/ssr/next.ssr.js
Did you mean to import next/navigation.js?
If I add resolve.conditions['browser']
to the config the error change and it can't seem to resolve something in the mock service worker setup.
Error: No known conditions for "./node" specifier in "msw" package
Then when I add server.deps.inline: true
Error: No known conditions for "./node" specifier in "msw" package
Serialized Error: { code: 'ERR_INSPECTOR_NOT_CONNECTED' }
For extra context here is my vitetest.config:
import { defineConfig } from 'vitest/config';
import react from '@vitejs/plugin-react';
import tsconfigPaths from 'vite-tsconfig-paths';
import graphql from '@rollup/plugin-graphql';
// https://vitejs.dev/config/
export default defineConfig({
plugins: [tsconfigPaths(), graphql(), react()],
resolve: {
// this line is important so the "browser build" of dependencies is used
// and not the "SSR build", which would contain "streaming-to-the-browser"
// specific code
conditions: ['browser'],
},
test: {
pool: 'forks',
globals: true,
environment: 'jsdom',
setupFiles: ['./config/vitest/setupTests.ts'],
server: {
// this is important so that the `graphql` dependency is inlined by vitest,
// which seems to get around the "dual package hazard" with ESM/CJS
// at least in this specific setup
deps: {
inline: true,
},
},
exclude: [...],
...,
},
},
});
However, these errors seem related to Vitest and MSW because because my setupTests.ts file starts a server to listen for API calls for MSW and by changing the conditions to browser, I don't think Vitest likes setupServer import since it runes in node: import { setupServer } from 'msw/node';.
This is problematic :/
msw
returns null
for that import in browser
(https://unpkg.com/msw@2.2.13/package.json), but on the other hand, you really should test the browser implementation.
The node
implementation of this package does all kind of stuff like trying to transport data over the wire into the browser, and you really don't want that to happen in your tests. It also changes the behaviour of all our hooks and will e.g. make useQuery
always skip: true
and change some of your fetch policies to values that make sense in a SSR, but not in a "live" environment.
I'll see if I can set up a chat with @kettanaito to talk this through.
@phryneas, I will be glad to chat about this. Conditional exports have caused a lot of pain to the community but I am convinced MSW describes them correctly. Multi-environment tooling is still hard.
The node implementation of this package does all kind of stuff like trying to transport data over the wire into the browser, and you really don't want that to happen in your tests.
I can propose a counter-argument: using a browser
module resolution for a Node.js process is broken by design. Since tools like JSDOM/HappyDOM never guaranteed full browser compatibility, resolving third-party entries to browser entries is destined to fail.
Even if we provide a dummy entrypoint like so:
{
"exports": {
"/node": { "browser": "./dummy.js" }
}
}
// dummy.js
throw new Error('Only use this in Node.js!')
It won't help, only obscure the mistake and make it harder to spot.
The problem here is the browser
module resolution used in a non-browser environment. That is a fundamental design flaw, and it could be fixed by some jsdom
/browser-like
conditional export to make things more apparent ("Hey, I'm pretending to be a browser here but I'm not really but let's imagine we resolve packages using their browser export").
I have a few thoughts on this. First, code meant for the browser must be tested in the browser. JSDOM has to go, I'm sorry. It's a broken tool by design and there's no reason not to use Playwright or Cypress nowadays. Second, if you absolutely must load a particular build of your app or a third-party, rather prefer using alias
than conditions
. I get the idea of wanting to test a browser build, not SSR build, but by setting conditions: ['browser']
you are affecting the entire module tree you import. That looks way out of scope of your original intention. Perhaps something like this would be more reasonable:
export default defineConfig({
resolve: {
alias: {
'dep-a': '/node_modules/dep-a/dist/index.browser.js',
'dep-b': '/node_modules/dep-b/dist/index.browser.js',
}
}
})
We've had a fantastic discussion with @phryneas and we've found a solution to this. He's working on the pull requests, I will review those and release the fix shortly. Stay tuned.
Thanks for all this quick collaboration!
Is this ready for me to test?
I bumped the packages: "msw": "^2.2.14"
and "@apollo/experimental-nextjs-app-support": "^0.0.0-commit-ef5e30b"
.
I reverted my vitest config to what it was before—removed resolve.conditions:['browser']
and server.deps.inline: true
Running tests gives this error output
Error: Cannot find module '/.../node_modules/next/navigation' imported from /.../node_modules/@apollo/experimental-nextjs-app-support/dist/ssr/next.ssr.js
Did you mean to import next/navigation.js?
Serialized Error: { code: 'ERR_MODULE_NOT_FOUND', url: 'file:///.../node_modules/next/navigation' }
You will still need resolve.conditions:['browser']
- it's just that MSW now supports that in vitest.
I am confused about that dist/ssr/next.ssr.js
file in your error message.
I'm pretty sure we never had a file with that name, I just went through a bunch of versions of the package.
Could you please change ^0.0.0-commit-ef5e30b
to 0.0.0-commit-ef5e30b
? The ^
will just pick up anything that is alphabetically higher, which could be any other pull request.
The changes are not ready yet. Wait for the next MSW minor release until trying anything. I need to wrap a few things and then the package will be there.
Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.
This has been released as 0.10.1 on our side, but we still need to verify the full story once the msw release is out, so keeping this open :)
msw@2.3.0 has been released, including the fix for this. Please update and let me know!
@phryneas @kettanaito it looks like this works! Thanks so much for jumping in here.
Just for context in case anyone reads this thread, I updated both packages and then added the browser resolve condition to my vitest.config.
// package.json
"@apollo/experimental-nextjs-app-support": "^0.10.1",
...
"msw": "^2.3.0",
// vitest.config.ts
export default defineConfig({
resolve: {
// This line is important so the "browser build" of dependencies is used
// and not the "SSR build", which would contain "streaming-to-the-browser"
// specific code
conditions: ['browser'],
},
Also worth mentioning, despite setting "browser"
in vitest.config.js
, Vite is smart and will use node
export condition as the fallback since the browser condition is an explicit null
for msw/node
. Keep that in mind. Not all tools have that fallback mechanism. If they don't you have to provide it yourself.
Hello!
We've been using
@apollo/experimental-nextjs-app-support
for a few months now with no issue; however, after the update fromv0.8.0
tov.0.9.0+
we have breaking unit tests with this error:Error: ApolloNextAppProvider cannot be used outside of the Next App Router!
This seems to be related to how we test components that require the ApolloWrapper/Provider. For context, we're using Mock Service Worker, Vitest, Next App router. We have a custom renderer function as a testing utility that allows us to wrap components in Providers that might be needed. In many of our cases we need to mount components with the ApolloWrapper so we can mock graphql calls inside our tests. Below I will add the error output that we are seeing. I will also post some code snippets and the versions we're running.
The error we see are these:
The strategy we use for simplifying the testing boilerplate for Wrappers/Providers comes right from the RTL docs for creating a custom renderer. This has worked thus far perfectly. After the version bump, this render function is throwing the error mentioned above.
When inside the unit test file, if I remove this custom render function and replace it with the RTL
render
function and I don't mount ApolloWrapper, my tests will pass, provided that I change the mocking structure of the network calls.In our tests look like this. All the tests that we have that require the ApolloWrapper/Provider fail with the error.
Here's what our ApolloProvider looks like for additional context. The implementation still seems to work locally. We're just having issues with our tests after the version.
Thanks! Let me know if I should add more context.