Conversation
Codecov Report
@@ Coverage Diff @@
## master #1195 +/- ##
=======================================
Coverage 97.48% 97.48%
=======================================
Files 9 9
Lines 398 398
Branches 123 123
=======================================
Hits 388 388
Misses 9 9
Partials 1 1 Continue to review full report at Codecov.
|
snitin315
force-pushed
the
fix-node-types
branch
from
ac374fe to
2f6d8e9
Compare
package.json
Outdated
| "webpack": "^4.0.0 || ^5.0.0" | ||
| }, | ||
| "dependencies": { | ||
| "@types/node": "^12.20.43", |
There was a problem hiding this comment.
I think we should avoid it, because it can be problem in future, ideally I think we can just copy statSync declaration in our types with todo comment
There was a problem hiding this comment.
- probably it will not be that simple because dev-middleware implicitly imports from
@types/nodeseveral other very complex types likeimport("http").IncomingMessage. Other examples:import("http").ServerResponseortypeof import("fs").readFileSync. - seems dependency should go to
devDependenciesand should allow newer major versions as well
would be great to move forward this PR because it blocks TS users from updating
There was a problem hiding this comment.
Why not update @types/node locally?
There was a problem hiding this comment.
it will be breaking change and will require minimal supported node version to be 16 for webpack-dev-middleware.
you can't safely use node@16 typings with node@12 codebase because it includes interface and feature changes from v16.
primary issue remains: webpack-dev-middleware does not list dependency it actually uses: @types/node@X.X.X
There was a problem hiding this comment.
Let's revisit this review @alexander-akait. What are we proposing as the better alternative solution here. If types are autogenerated it doesn't make sense to patch the statSync declaration.
There was a problem hiding this comment.
Do we add it as devDependency here in webpack-dev-middleware or webpack-dev-server?
There was a problem hiding this comment.
@snitin315 - here, in the webpack-dev-middleware package.
There was a problem hiding this comment.
Oh looks like I mistyped earlier - yeah I meant webpack-dev-middleware, not webpack-dev-server
There was a problem hiding this comment.
Thank you for the contributions here 💯
|
We have a blocker #1260, hope we get patch in near future and I will do release |
5 participants
This PR contains a:
Motivation / Use-Case
Fix #1194
Breaking Changes
None
Additional Info
No