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 nodejs typing #281

Merged
merged 7 commits into from
Dec 7, 2023
Merged

Fix nodejs typing #281

merged 7 commits into from
Dec 7, 2023

Conversation

mhaligowski
Copy link
Contributor

@mhaligowski mhaligowski commented Nov 10, 2023

Current types definition for the node-mocks-http library extend the HTTP requests and responses from express. As a result, HTTP request/response implementations from other frameworks (e.g. NextApiRequest) are type-incompatible with node-mocks-http.

This changes the generic class of the mock objects from the Request/Response from express library to the NodeJS IncomingMessage/OutgoingMessage in the type definition. Unless specified otherwise, the default generic type is still the express object. It's inheriting from the NodeJS object, so it works fine. As a result, unless explicitly specified, there won't be any compatibility issues.

UPDATE:

Adding TypeScript tests turned out a little more complicated than I initially expected, hence only the one for mockRequest. In the process of writing them I ran into couple of issues:

  • unnecessary declare module, IIUC it is only needed when writing types declaration for external modules, not for within the same package,
  • the definitions file needed to be renamed,
  • added tsd NPM package, which specifically tests the types.

@eugef
Copy link
Owner

eugef commented Nov 14, 2023

Hi @mhaligowski thanks for the fix.

I wonder, how it will work in case Express-specific methods or properties are used.

For example:

const res = createResponse();
res.locals = {foo: 'bar'}

Since .locals is not part of OutgoingMessage type, won't it become an Typescript error?

@mhaligowski
Copy link
Contributor Author

Hi @mhaligowski thanks for the fix.

I wonder, how it will work in case Express-specific methods or properties are used.

For example:

const res = createResponse();
res.locals = {foo: 'bar'}

Since .locals is not part of OutgoingMessage type, won't it become an Typescript error?

In the simple form, i.e. const res = createResponse(), the default return type is still Mock<Response>. However, with my change it's possible to call for example const res = createResponse<NextApiResponse>(), and the mock type will be Mock<NextApiResponse>. In the current implementation the NextApiResponse is not accepted by TypeScript because it doesn't extend express.Response class.

I think that the best way will be to just a TypeScript test to the test suite, but I don't really want to mess it up too bad with this PR. Let me see if I can modify the test routine so that we can start adding TypeScript tests as well.

@mhaligowski
Copy link
Contributor Author

@eugef I added a PR with capability of writing TS test! Let me know if it's fine, and I'll rebase this PR on top of the TypeScript tests and will add some tests to make sure the current change works fine!

@eugef
Copy link
Owner

eugef commented Nov 27, 2023

Hi @mhaligowski PR #282 looks good to me.

Feel free to proceed and add some tests.

@mhaligowski
Copy link
Contributor Author

Phew! Those tests were bigger than expected! Let me update the PR body with the explanation what I did.

Copy link
Owner

@eugef eugef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mhaligowski, big thanks for the tremendous amount of work you've put in!

This PR looks solid from my end. If you believe it's ready to be merged, I'll go ahead, and release a new minor version.

@mhaligowski
Copy link
Contributor Author

Hey @mhaligowski, big thanks for the tremendous amount of work you've put in!

This PR looks solid from my end. If you believe it's ready to be merged, I'll go ahead, and release a new minor version.

I think so, although it's still my first time messing with .d.ts files. Would it make sense to have a pre-release? I don't want to risk breaking the package. The code is tested, but not sure if there's test coverage for package.

@eugef
Copy link
Owner

eugef commented Dec 6, 2023

I'd suggest to make a pre-relase locally just for yourself and try this new version in some of your TS projects.
I think this would be sufficient check

@mhaligowski
Copy link
Contributor Author

I'd suggest to make a pre-relase locally just for yourself and try this new version in some of your TS projects. I think this would be sufficient check

Just did that!

I ran npm pack with a changed version, then imported into my project with "file://path/to/file.tgz. The problem magically disappeared :)

Good to go from me!

@eugef
Copy link
Owner

eugef commented Dec 7, 2023

Great, thanks for testing.

Could you also add a section to readme that explains how to use this lib with NextApiRequest and TS

@eugef eugef merged commit 8e92d5a into eugef:master Dec 7, 2023
3 checks passed
@mhaligowski
Copy link
Contributor Author

Could you also add a section to readme that explains how to use this lib with NextApiRequest and TS

Opened #284!

@mhaligowski mhaligowski deleted the fix-nodejs-typing branch December 11, 2023 02:48
@Meags27
Copy link

Meags27 commented Dec 30, 2023

Is it possible to add documentation on how to use NextResponse with next.js's App router? as I keep getting TypeError: request.json is not a function as an error

const mockRequestData = { emailToUpdate: "test@example.com" };
    const { req, res } = createMocks({
      method: "POST",
      url: "/api/account/email/save",
      body: mockRequestData,
    });
  try {
    const { emailToUpdate } = await request.json();
   
    return NextResponse.json({ reauthenticated: true }, { status: 200 });
  } catch (error) {
    console.error("Error updating Cognito email", error);
  }
}

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.

3 participants