fix: resolve segfault issue caused by "node:events" import #1885
Conversation
| import * as path from 'node:path' | ||
| import { defineConfig } from 'vitest/config' | ||
|
|
||
| export default defineConfig({ |
There was a problem hiding this comment.
This seems to be the exact copy of the existing vitest config for node tests. Cannot we just reference to it for native tests, which I assume is the intention here?
There was a problem hiding this comment.
yes we probably can. I was separating this to try and get a test environment that worked properly for react-native but failed to do so, and kept this to provide a path to doing that in the future. I think it's probably ok to duplicate here, but we could also nest the native test inside of the node folder and just run it there, since it's fairly small.
No strong preferences, although a working native setup would be ideal. I couldn't even get vi.mock to throw for builtins here, which I also tried
There was a problem hiding this comment.
I made the vi.mock() work but it requires a module alias hack in Vitest.
src/node/SetupNodeServerApi.ts
Outdated
| import { SetupServer } from './glossary' | ||
| import { isNodeExceptionLike } from './utils/isNodeExceptionLike' | ||
|
|
||
| export class SetupNodeServerApi extends SetupServerApi implements SetupServer { |
There was a problem hiding this comment.
I like this approach. A couple of thoughts to make it stellar:
- Don’t introduce a new class.
- Add a protected
onRequestmethod to the existingSetupServerApiclass. Do nothing in that method. - In
src/node/index.ts, create a new subclass fromSetupServerApi. In that subclass, invoke the abort listener logic directly (no need to abstract to a method). This way, the import tonode:eventswill be scoped tonode/index.tsand won’t leak to/nativeonSetupServerApi.tsimports there. - No changes are necessary for the React Native setup API.
So, this is basically identical to what you did just inlining the new derived class directly in src/node/index.ts. What do you think about this?
There was a problem hiding this comment.
My main concern here is that SetupApi classes are meant to cover functionality. Here, we sort of address the extension issue that's import-specific (if we import node:events in the base SetupApi, any consumer automatically gets that import, which is not what we want). An internal issue.
There was a problem hiding this comment.
yep, makes sense.
1e44a5b might be what you're suggesting (just keeping from a separate file?)
kettanaito
changed the title
| * The root-level package.json is still CJS, which, I suspects, | ||
| * resolves any subsequent in-build imports to their CJS counterparts. | ||
| */ | ||
| expect(() => setupServer()).not.toThrow() |
There was a problem hiding this comment.
I've added a very basic test that due to the lack of proper testing environment, tests implementation detail of the node:events import (or its lack) in the msw/native build.
| @@ -14,6 +14,6 @@ test('throws an error given an Array of request handlers to setupServer', () => | |||
| } | |||
|
|
|||
| expect(createServer).toThrow( | |||
| `[MSW] Failed to construct "SetupServerApi" given an Array of request handlers. Make sure you spread the request handlers when calling the respective setup function.`, | |||
| `[MSW] Failed to construct "SetupNodeServerApi" given an Array of request handlers. Make sure you spread the request handlers when calling the respective setup function.`, | |||
There was a problem hiding this comment.
I don't like the subclass name leaking to the consumer. We should somehow keep this as SetupServerApi.
| * the root-level "package.json" is not "module". | ||
| */ | ||
| 'msw/node': path.resolve(LIB_DIR, 'node/index.mjs'), | ||
| 'msw/native': path.resolve(LIB_DIR, 'native/index.mjs'), |
There was a problem hiding this comment.
@mattcosta7, this is the hack that makes vi.mock() work with overriding the standard node modules.
There was a problem hiding this comment.
This looks good. Thanks for fixing this, @mattcosta7! I've addressed the remaining feedback here and it should be ready for merge.
Released: v2.0.9 🎉This has been released in v2.0.9! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
A 3rd attempt which uses a subclass to implement the node specific behavior, instead of the methods in:
SetupServerApifirst argument in constructor #1880I think this is my favorite implementation, and by far the least leaky, while still allowing control of the imports outside of the class definition, which allows us to better utilize this in only the node export
I still haven't managed to recreate a proper test environment that actually throws, but we're at least avoidng the codepath here