refactor: use native req/res methods #747

privatenumber · 2020-10-27T10:12:42Z

This PR contains a:

Motivation / Use-Case

The code was using express specific methods for very simple things (eg. getting/setting header).

I use connect to keep my server light-weight, and others may be using frameworks like koa.

This change refactors the express method calls to use native Node.js HTTP request/response methods instead.

Breaking Changes

Additional Info

codecov · 2020-10-27T10:14:10Z

Codecov Report

Merging #747 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #747      +/-   ##
==========================================
+ Coverage   99.15%   99.16%   +0.01%     
==========================================
  Files          10       10              
  Lines         236      239       +3     
  Branches       72       73       +1     
==========================================
+ Hits          234      237       +3     
  Misses          2        2

Impacted Files	Coverage Δ
src/middleware.js	`100.00% <100.00%> (ø)`
src/utils/handleRangeHeaders.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9230c13...69a740a. Read the comment docs.

alexander-akait

We need to setup tests for connect in this case, we can't just change code because something use it, it can breaking other compatibles

alexander-akait · 2020-10-27T12:14:39Z

I can help you with it, should be not hard

ylemkimon · 2020-10-27T15:03:25Z

Note this is a revert of #726.

alexander-akait · 2020-10-27T15:06:33Z

Yes, so I ask to add tests and we should create compatibility.js file and store these hacks there

privatenumber · 2020-10-27T23:30:26Z

I can wrap the entire middleware.test.js with an iteration so they all run for express and connect, but this might add a lot of time to tests because it's a very large test file.

What do you think of just swapping express with connect in the test environment?

Since connect is just a middleware framework and doesn't add custom request/response methods like express does, using the underlying native HTTP request/response methods and testing with connect will also mean it works for express. (middlewares designed for connect can always be used by express, eg. compression).

alexander-akait · 2020-10-28T10:29:49Z

What do you think of just swapping express with connect in the test environment?

Bad idea, we should we should test on what it works on

Since connect is just a middleware framework and doesn't add custom request/response methods like express does, using the underlying native HTTP request/response methods and testing with connect will also mean it works for express. (middlewares designed for connect can always be used by express, eg. compression).

yes, but we need to add it for all tests, otherwise we can't guarantee compatibility

privatenumber · 2020-10-28T17:20:32Z

How would you like to add tests for both? (eg. should I wrap the entire middleware tests so they run for both express and connect?)

Also, supporting connect is effectively supporting a vanilla Node.js HTTP server as well, if you're interested in adding tests for that as well.

alexander-akait · 2020-10-28T17:28:50Z

How would you like to add tests for both? (eg. should I wrap the entire middleware tests so they run for both express and connect?)

Ideally - yes

Also, supporting connect is effectively supporting a vanilla Node.js HTTP server as well, if you're interested in adding tests for that as well.

Yes 👍

…to native-req-res

privatenumber · 2020-10-29T06:22:02Z

@evilebottnawi

I added an outer iteration in the middleware test so that they're executed for connect and express. There's a few tests that fail though, but only when I run them both. If I comment out either connect or express they all succeed.

I couldn't find out what was causing this. Do you mind looking into this?

Also looked into adding a vanilla HTTP server, but I may have over-spoke. Adding middleware support wasn't as easy as I imagined... the whole connect module is doing that after all, so probably not necessary anyway.

BTW I still think it's kinda pointless to test for express if we're testing with connect, and we're not using any express features. At most, I think just add a smaller express smoke test. Maybe you'll agree as you checkout the code.

ylemkimon · 2020-10-29T07:11:38Z

BTW I still think it's kinda pointless to test for express if we're testing with connect, and we're not using any express features. At most, I think just add a smaller express smoke test. Maybe you'll agree as you checkout the code.

webpack-dev-server uses express for the framework.

privatenumber · 2020-10-29T07:32:46Z

Thanks for chiming in @ylemkimon

This PR removes dependency on express-only API so that it can be HTTP framework agnostic. Instead of using the express API, this middleware now only uses native Node.js HTTP API. Therefore, this middleware now works with any HTTP framework that expose native request/response instances because we're no longer interacting with any framework code. The only dependency now is the "HTTP middleware paradigm".

You can confirm this by swapping express with connect in the tests. They will all pass because the code is now framework agnostic.

connect is just the "middleware" feature of express, and because of its minimal approach, it makes it easier to test and keep a stricter scope. For example, express automatically adds etags, which interferes with testing the etag feature that I'm working on here.

So, testing with express in addition to connect is almost like, making a module that interacts with the DOM, and testing it with Preact and React via mounted hook... as long as it's interacting with the DOM, it's going to work so you don't need to test it with every JS framework out there.

To support this even further, just take a look at one of the most popular express middleware compression by the express team. It doesn't even use express or connect to test, it just uses a simple HTTP server.

If we want first-class support for express just in case, I think adding a smoke test for it will be sufficient.

alexander-akait · 2020-10-29T11:45:42Z

src/middleware.js

+        res.setHeader(
+          'Content-Type',
+          contentType || 'application/octet-stream'
+        );


We should always use official middleware API, so for example we shoudl use express API, for connect (no example API) we should use http module API, why? Because other middlewares/tools can add additional logic for built-in API, and we will not working properly with them

So we need to create compatibility file and put some utils there, like:

function responseGetHeader(response, header) { // Comment there we describe what it function belong express API and some more interesting stuff if (response.get(header)) { // Logic } // Other logic }

I agree with your explanation, and that's why this middleware can't be used by Koa, even though they expose native request/response.

However, if we scope our support level to vanilla HTTP, express, connect, and other frameworks that don't add extra logic, then we don't need to do that. We can't possibly support all frameworks anyway.

So let's take my suggestion, it is easy and will not confusing for future refactoring

If the support level is strictly express and connect, I think it's unnecessary.

Going back to the case with compression, I'm not convinced why a third-party middleware requires this abstraction when a first-party middleware doesn't.

Can you clarify?

clarkdo · 2020-10-29T22:36:50Z

Thanks for this pr, look forward to the release.

privatenumber · 2020-10-31T23:09:44Z

@evilebottnawi

Following up with #747 (comment)

I couldn't find out what was causing this. Do you mind looking into this?

alexander-akait · 2020-11-09T14:19:10Z

Okay, I am start to working on it, want to separate it on two commits - prepare tests + compatibility fixes to avoid dirty git history

alexander-akait · 2020-11-09T19:33:00Z

Fixed #760

@privatenumber big thanks for helping and investigating, I will do release tomorrow, the next target - ETag header and maybe other cache headers

privatenumber · 2020-11-09T22:43:56Z

Thanks for getting it in @evilebottnawi. Looking forward to being able to use it!

Not a big deal because I primarily did this for the feature, but for future reference, I would appreciate it if my commits could've somehow been integrated into your PR and the repository. I put in a lot of hours into this but I won't be credited as a contributor.

refactor: use native req/res methods

320a1fa

privatenumber requested review from alexander-akait and hiroppy as code owners October 27, 2020 10:12

alexander-akait requested changes Oct 27, 2020

View reviewed changes

fix: tests and res.status method

52ac112

privatenumber added 2 commits October 29, 2020 02:16

test(middleware): add connect

03cfa9b

Merge branch 'master' of github.com:webpack/webpack-dev-middleware in…

69a740a

…to native-req-res

privatenumber mentioned this pull request Oct 29, 2020

feat: etag support #746

Closed

6 tasks

alexander-akait reviewed Oct 29, 2020

View reviewed changes

alexander-akait mentioned this pull request Oct 29, 2020

4.0.0-rc.1 is incompatible with Fastify #602

Closed

clarkdo mentioned this pull request Oct 29, 2020

chore(deps): update dependency webpack-dev-middleware to v4 nuxt/nuxt#8260

Closed

1 task

morlay mentioned this pull request Nov 2, 2020

@querycap-dev/webpack-browser-sync upgrade webpack-dev-middleware 4.0.0 querycap/webappkit#50

Closed

2 tasks

alexander-akait mentioned this pull request Nov 4, 2020

add BrowserSync support #758

Closed

alexander-akait closed this Nov 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use native req/res methods #747

refactor: use native req/res methods #747

privatenumber commented Oct 27, 2020

codecov bot commented Oct 27, 2020 •

edited

Loading

alexander-akait left a comment

alexander-akait commented Oct 27, 2020

ylemkimon commented Oct 27, 2020

alexander-akait commented Oct 27, 2020

privatenumber commented Oct 27, 2020

alexander-akait commented Oct 28, 2020

privatenumber commented Oct 28, 2020

alexander-akait commented Oct 28, 2020

privatenumber commented Oct 29, 2020 •

edited

Loading

ylemkimon commented Oct 29, 2020

privatenumber commented Oct 29, 2020 •

edited

Loading

alexander-akait Oct 29, 2020

alexander-akait Oct 29, 2020 •

edited

Loading

privatenumber Oct 31, 2020

alexander-akait Oct 31, 2020

privatenumber Oct 31, 2020

alexander-akait Nov 2, 2020

clarkdo commented Oct 29, 2020

privatenumber commented Oct 31, 2020

alexander-akait commented Nov 9, 2020

alexander-akait commented Nov 9, 2020

privatenumber commented Nov 9, 2020

refactor: use native req/res methods #747

refactor: use native req/res methods #747

Conversation

privatenumber commented Oct 27, 2020

Motivation / Use-Case

Breaking Changes

Additional Info

codecov bot commented Oct 27, 2020 • edited Loading

Codecov Report

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait commented Oct 27, 2020

ylemkimon commented Oct 27, 2020

alexander-akait commented Oct 27, 2020

privatenumber commented Oct 27, 2020

alexander-akait commented Oct 28, 2020

privatenumber commented Oct 28, 2020

alexander-akait commented Oct 28, 2020

privatenumber commented Oct 29, 2020 • edited Loading

ylemkimon commented Oct 29, 2020

privatenumber commented Oct 29, 2020 • edited Loading

alexander-akait Oct 29, 2020

Choose a reason for hiding this comment

alexander-akait Oct 29, 2020 • edited Loading

Choose a reason for hiding this comment

privatenumber Oct 31, 2020

Choose a reason for hiding this comment

alexander-akait Oct 31, 2020

Choose a reason for hiding this comment

privatenumber Oct 31, 2020

Choose a reason for hiding this comment

alexander-akait Nov 2, 2020

Choose a reason for hiding this comment

clarkdo commented Oct 29, 2020

privatenumber commented Oct 31, 2020

alexander-akait commented Nov 9, 2020

alexander-akait commented Nov 9, 2020

privatenumber commented Nov 9, 2020

codecov bot commented Oct 27, 2020 •

edited

Loading

privatenumber commented Oct 29, 2020 •

edited

Loading

privatenumber commented Oct 29, 2020 •

edited

Loading

alexander-akait Oct 29, 2020 •

edited

Loading