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

Express 5 types released - deduplication problem #307

Closed
RobinTail opened this issue Oct 2, 2024 · 9 comments · Fixed by #309
Closed

Express 5 types released - deduplication problem #307

RobinTail opened this issue Oct 2, 2024 · 9 comments · Fixed by #309

Comments

@RobinTail
Copy link
Contributor

RobinTail commented Oct 2, 2024

Good news: @types/express@5.0.0 released.
However, this line leads to multiple versions installed on consumer side:

"@types/express": "^4.17.21",

Would it be possible somehow to address this?

An approach could be moving @types/express to peerDependencies (and probably devDependencies for building purposes). So that consumer would decide which version of @types/express to use accordingly.

I also noticed, that those types are only used to define defaults for RequestType. Another approach could be removing defaults.

Third option could be defining dependency with >=or as *, which however would lead to v5 by default, that can consequently by corrected by resolution/overrides entry on consumer side.

What do you think, @eugef ?

@RobinTail RobinTail changed the title Express 5 types compatibility Express 5 types released - deduplication problem Oct 2, 2024
@eugef
Copy link
Owner

eugef commented Oct 2, 2024

Hi @RobinTail, if it easy to remove @types/express dep - let's do this, otherwise I'd go for the peerDependencies approach.

@RobinTail
Copy link
Contributor Author

Ok. Then I propose making a conceptual change to the following idea:

Unless specified explicitly, they will be return an Express-based request/response

(it's from Readme)

The typings for TypeScript are bundled with this project. In particular, the `.createRequest()`, `.createResponse()` and `.createMocks()` methods are typed and are generic. Unless specified explicitly, they will be return an Express-based request/response object:

so that it would be different:

  • You SHOULD specify the type of request/response object of your framework;
  • Otherwise the following types are used by default: http.IncomingMessage and http.OutgoingMessage

(wording is a subject to collaborate on).

That would allow to detach from @types/express and delegate to consumer supplying an explicit type according to the installed version of express and @types/express.

I will try my best on making a corresponding PR, @eugef

@eugef
Copy link
Owner

eugef commented Oct 3, 2024

This sounds to me like a backwards incompatible change - let's not go this way.

I am fine with moving @types/express to the peerDependencies

@RobinTail
Copy link
Contributor Author

I am fine with moving @types/express to the peerDependencies

That would also be a breaking change, @eugef

@RobinTail
Copy link
Contributor Author

But it should not be scary, I presume, making a next major version slightly different.
What do you think?

@RobinTail
Copy link
Contributor Author

I am fine with moving @types/express to the peerDependencies

Yeah, I can make an alternative implementation using this approach, so you could compare them...
Stand by :)

@eugef
Copy link
Owner

eugef commented Oct 3, 2024

I am fine with moving @types/express to the peerDependencies

That would also be a breaking change, @eugef

It would be much less "breaking" change.

For many projects node-mocks-http is not the only package that depends on @types/express so they won't notice any difference, otherwise npm and yarn will show a warning.

BTW, for consistency let's also move @types/node to peerDependencies as well

@RobinTail
Copy link
Contributor Author

Check this out, @eugef :
#309

It's actually really prettier and smaller change.
For those using pure JavaScript nothing will change at all.
TypeScript users would have to install their own @types/express and @types/node.

@eugef eugef closed this as completed in #309 Oct 4, 2024
@eugef
Copy link
Owner

eugef commented Oct 4, 2024

@RobinTail, thanks for your contribution,
This fix is published in version 1.16.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants