feat(middleware): add `content` option (`options.content.filter`, `options.content.transform`) #336

ferdinando-ferreira · 2018-08-29T01:24:51Z

What kind of change does this PR introduce?
Feature

Did you add tests for your changes?
Yes

Summary
It's a refactoring of the functionality proposed at #330 with the requested changes implemented

Added explicit "NotFound" flag, will be used in the subsequent features

ferdinando-ferreira · 2018-08-29T01:29:38Z

lib/middleware.js

+          if (!context.options.content.filter(currentRequest)) {
+            return goNext();
+          }
+        }
        if (notFound) {
          return resolve(goNext());


Question: why is it that every other "goNext" is a regular return but this one is a return resolve(goNext())?

Good question but I don't know the answer, try to remove the resolve and see what's happening (if something happens). Please add a comment that this is something to investigate and clearify :)

ferdinando-ferreira · 2018-08-29T01:31:07Z

lib/middleware.js

+          if (notFound) {
+            return localResolve(null);
+          }
+          fs.readFile(localRequest.filename, (err, content) => {


Is it a problem that the readFileSync was replaced by an async one?

I can't say to 💯, but async I/O should be preferred. Maybe add a comment that it was changed in case we need to (temp) revert this for some reason :)

fs is a in memory filesystem so it doesn't matter

ferdinando-ferreira · 2018-08-29T01:33:09Z

Missing: changing the README.md, working on it but it would gain some time to request comments on the code itself meanwhile.

michael-ciniawsky · 2018-08-29T02:37:36Z

lib/middleware.js

-        // do not add charset to WebAssembly files, otherwise compileStreaming will fail in the client
-        if (!/\.wasm$/.test(filename)) {
-          contentType += '; charset=UTF-8';
+        const currentRequest = Object.assign({}, {


const currentRequest => const request (we don't have different/multiple request(s) in this context)

michael-ciniawsky · 2018-08-29T02:41:56Z

lib/middleware.js

+          if (notFound) {
+            return localResolve(null);
+          }
+          fs.readFile(localRequest.filename, (err, content) => {


I can't say to 💯, but async I/O should be preferred. Maybe add a comment that it was changed in case we need to (temp) revert this for some reason :)

michael-ciniawsky · 2018-08-29T02:46:44Z

lib/middleware.js

        }
+        const defaultTransform = (localRequest, fs) => new Promise((localResolve) => {


Why localRequest && localResolve instead of just req(uest) && resolve ? If it is because of the linter feel free to add a e.g /* eslint-disable no-shadow */ to the top of the file :)

michael-ciniawsky · 2018-08-29T02:57:45Z

lib/middleware.js

+            }
+          });
+        });
+        const optionTransform = (context.options && context.options.content) ? context.options.content.transform : null;


hmmm I not sure 💯 here, but something in the direction of

const { options, fs } = context // readFile -> Object { request, content, fs } const readFile = (request, fs) => new Promise((resolve) => { ... return { request, content, fs } }) // transform -> Promise<String|?Buffer> const transform = (options.content && options.content.transform) ? options.content.transform : ({ request, content, fs }) => content // default return readFile(request, fs) .then(transform) .then((content) => { ... })

maybe, but definitely open for suggestions

michael-ciniawsky · 2018-08-29T03:02:29Z

test/tests/server.js

+      instance = middleware(compiler, {
+        stats: 'errors-only',
+        content: {
+          filter(currentRequest) {


currentRequest => res || request

michael-ciniawsky · 2018-08-29T03:04:15Z

lib/middleware.js

@@ -38,6 +38,7 @@ module.exports = function wrapper(context) {
    return new Promise(((resolve) => {
      handleRequest(context, filename, processRequest, req);
      function processRequest() {
+        let notFound;


Please elaborate further on the need for this

michael-ciniawsky · 2018-08-29T03:06:15Z

lib/middleware.js

-              res.setHeader(name, context.options.headers[name]);
+            const { headers } = context.options;
+            if (headers) {
+              for (const name in headers) {


name => header

michael-ciniawsky · 2018-08-29T03:07:58Z

lib/middleware.js

+          .then(content => ((!optionTransform) ? content : optionTransform(currentRequest, content, context.fs)))
+          .then((content) => {
+            if (content == null) {
+              return Promise.reject();


Should we better reject with an {Error} here ?

if (content == null) { const err = new Error('<message>') return Promise.reject(err) }

michael-ciniawsky · 2018-08-29T03:14:34Z

Missing: changing the README.md, working on it but it would gain some time to request comments on the code itself meanwhile.

Yep, let's discuss the implementation first and once approved you can add the docs then :)

michael-ciniawsky · 2018-08-29T03:21:50Z

This overall looks very good :)

How in your opinion could this potentially replace options.serverSideRendering in the future? What is (eventually) still missing to do so at the moment?

codecov · 2018-08-29T03:27:39Z

Codecov Report

Merging #336 into master will decrease coverage by 0.18%.
The diff coverage is 93.93%.

@@            Coverage Diff             @@
##           master     #336      +/-   ##
==========================================
- Coverage   96.82%   96.64%   -0.19%     
==========================================
  Files           7        7              
  Lines         252      268      +16     
==========================================
+ Hits          244      259      +15     
- Misses          8        9       +1

Impacted Files	Coverage Δ
lib/middleware.js	`94.11% <93.93%> (-0.12%)`	⬇️

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 610e260...14e9ecd. Read the comment docs.

alexander-akait · 2018-08-29T10:11:47Z

@michael-ciniawsky can we describe this feature in main post briefly, it is hard to read a lot of posts?

ferdinando-ferreira · 2018-08-29T13:11:04Z

can we describe this feature in main post briefly, it is hard to read a lot of posts?

Here is the shortest summary:

options.content.filter: receives a function with the following arguments: (request). This function is called once for each resource being served by webpack-dev-middleware. Returning false makes webpack-dev-middleware not to serve said file and pass it along to the next middleware.
options.content.transform: receives a function with the following arguments: (request, content, memoryFs). This function is called once for each resource being served by webpack-dev-middleware and must return a promise whose resolve contains the Buffer with the actual content to be served and whose reject makes webpack-dev-middleware not to serve said file and pass it along to the next middleware.

The added tests contain examples of use cases for these options and, in general, they supercede what's being offered by options.serverSideRender, but in a more generic way.

alexander-akait · 2018-08-29T13:22:20Z

@ferdinando-ferreira do you need server side rendering for development env?

Also please provide you stack for ssr (frameworks, react/vue/etc, simple example, better repo)

ferdinando-ferreira · 2018-08-29T14:24:03Z

do you need server side rendering for development env?

It is my belief that the test environment should be as close if not an exact replica of the production one including all features. That makes testing the rendering of the project with SSR important as it can showcase eventual inconsistencies to be fixed during development.

Also please provide you stack for ssr (frameworks, react/vue/etc, simple example, better repo)

Here is an example I prepared to showcase the full power of the proposed solution. It is a fully featured turnkey hot reloaded server side rendered single file component powered environment to develop vuejs applications.
That is just one example tho, the new option in itself does not have any dependency or even hard coupling with Vue, SSR, it has many other uses other than what's being shown here. It solves an unsolved problem that is

what if one wants to dynamically change the served resource without the need of a recompile?

As of now (as it can be seen at #306) the way to properly implement SSR consists in essentially to tell webpack-dev-middleware not to serve a certain file (in a roundabout way, by passing a "falsy" value on the index field) and, then, to serve it in a subsequent middleware.
The new option makes it unnecessary because it can instruct webpack-dev-middleware to transform the content to modify it as required (using content.transform) or to explicitly tell it to skip serving the file (with content.filter).

git clone --single-branch -b samples https://github.com/ferdinando-ferreira/webpack-dev-middleware.git
cd webpack-dev-middleware
npm install
cd samples
cd vue-ssr
npm install
npm run dev

Here is the commit with the sample code referenced above.

alexander-akait · 2018-08-29T15:18:16Z

@ferdinando-ferreira

sokra [6:10 PM]
hmm.. not sure if this is needed. It's a middleware, so you could create another middleware to transform the responses. And anyway transformations to assets can be done as webpack plugin...

What do you think about this solution?

ferdinando-ferreira · 2018-08-29T17:30:07Z

What do you think about this solution?

@evilebottnawi: There are two components in the suggestion

And anyway transformations to assets can be done as webpack plugin...

That is not accurate for the use case proposed for this PR. Any transformation done through plugins or loaders only happen in compile time, making it necessary to restart the server and recompile for it any change to it to have effect.
With the proposed change that becomes similar to "hot reload", meaning refreshing the page would immediately reflect the modification.

Here is an example: in the code mentioned at #336 (comment), while running its development server through npm run dev), you can edit samples/vue-ssr/src/components/App.vue and

add a new route
add a new pointing to the above mentioned route, which appears automatically in the page without any refresh due to HMR
click said link which will correctly show the new route without the need for a compilation (considering it happens automatically due to the transform option)

Without the middleware level transform (doing it with a plugin, for instance) would make necessary to stop the dev server, start the dev server for the route change to take effect.

Here is an example. Edit the following file samples/vue-ssr/src/components/App.vue:

Change

    <ul class="links">
      <router-link :to="{ name: 'home' }" tag="li"><a>Home</a></router-link>
      <router-link :to="{ name: 'about' }" tag="li"><a>About</a></router-link>
      <router-link to="/nopage" tag="li"><a>Missing page</a></router-link>
      <router-link to="/nopage2" tag="li"><a>Missing page 2</a></router-link>
    </ul>

with

    <ul class="links">
      <router-link :to="{ name: 'home' }" tag="li"><a>Home</a></router-link>
      <router-link :to="{ name: 'about' }" tag="li"><a>About</a></router-link>
      <router-link to="/nopage" tag="li"><a>Missing page</a></router-link>
      <router-link to="/nopage2" tag="li"><a>Missing page 2</a></router-link>
      <router-link :to="{ name: 'home2' }" tag="li"><a>Home 2</a></router-link>
    </ul>

and

  export const routes = [
    { name: 'home', path: '/', component: Home, meta: { title: 'Homepage' } },
    { name: 'about', path: '/about', component: About, meta: { title: 'About' } },
    { name: 'notfound', path: "*", component: NotFound, meta: { title: 'Not Found' } }
  ];

with

  export const routes = [
    { name: 'home', path: '/', component: Home, meta: { title: 'Homepage' } },
    { name: 'about', path: '/about', component: About, meta: { title: 'About' } },
    { name: 'notfound', path: "*", component: NotFound, meta: { title: 'Not Found' } },
    { name: 'home2', path: '/home2', component: Home, meta: { title: 'Homepage 2' } }
  ];

Refresh the page (HMR is not working properly, product of a soon to be filed bug with webpack-serve, which only uses the "serve" property of the first configuration in a multiconfig env) and see that the newly created route becomes available immediately.

To the best of my knowledge there is no way to replicate this behaviour with the use of plugins because they're invoked in compile time while the middleware is invoked in page load time.

It's a middleware, so you could create another middleware to transform the responses.

The largest obstacle here (the one this new option is trying to solve) is the fact webpack-dev-server (when serverSideRender is set) does not expose the memory FileSystem to the next middleware (like it does with webpackStats, via res.locals).
I can propose a smaller change to middleware.js that passes fs along with webpackStats to the next middleware in line but the new option as proposed not only solves the SSR but in a much more generic way, not tied either to SSR or to any particular implementation.

Please confirm if this option has a chance of getting approved so I can work on the changes requested by @michael-ciniawsky or table the request.

Best regards

ferdinando-ferreira · 2018-08-30T13:44:04Z

I can propose a smaller change to middleware.js that passes fs along with webpackStats to the next middleware in line

Done, PR #337. Both PRs solve the same issue, this one in a more generic fashion, the other one contingent on ServerSideRender flag not going away.

ferdinando-ferreira · 2018-09-03T08:13:51Z

Rescinding this PR, #337 is enough as long as ServerSideRequest doesn't get dropped.

michael-ciniawsky · 2018-09-03T08:30:53Z

We will definitely keep it until we found a equally sufficient replacement for SSR workflows. I will review #337 asap. Thx for your efforts on this so far :)

ferdinando-ferreira added 3 commits August 28, 2018 18:48

Added explicit "NotFound" flag

c5b8551

Added explicit "NotFound" flag, will be used in the subsequent features

Added option: content.filter

425c27b

Added option: content.transform

14e9ecd

ferdinando-ferreira requested review from alexander-akait and michael-ciniawsky as code owners August 29, 2018 01:24

ferdinando-ferreira commented Aug 29, 2018

View reviewed changes

michael-ciniawsky reviewed Aug 29, 2018

View reviewed changes

michael-ciniawsky added this to the 3.3.0 milestone Aug 29, 2018

michael-ciniawsky added priority: 3 (important) severity: 4 (inconvenient) semver: minor type: feature pr: review status: discuss labels Aug 29, 2018

alexander-akait requested review from sokra and ooflorent August 29, 2018 14:32

ferdinando-ferreira mentioned this pull request Aug 30, 2018

feat(middleware): expose the memory filesystem (response.locals.fs) #337

Merged

ferdinando-ferreira closed this Sep 3, 2018

michael-ciniawsky added status: declined and removed pr: review priority: 3 (important) severity: 4 (inconvenient) status: discuss labels Sep 3, 2018

michael-ciniawsky removed this from the 3.3.0 milestone Sep 3, 2018

michael-ciniawsky removed request for ooflorent and sokra September 3, 2018 08:31

		}
		const defaultTransform = (localRequest, fs) => new Promise((localResolve) => {

Uh oh!

feat(middleware): add content option (options.content.filter, options.content.transform) #336

feat(middleware): add content option (options.content.filter, options.content.transform) #336

Uh oh!

Conversation

ferdinando-ferreira commented Aug 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ferdinando-ferreira commented Aug 29, 2018

Uh oh!

michael-ciniawsky Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-ciniawsky commented Aug 29, 2018

Uh oh!

codecov bot commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alexander-akait commented Aug 29, 2018

Uh oh!

ferdinando-ferreira commented Aug 29, 2018

Uh oh!

alexander-akait commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ferdinando-ferreira commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexander-akait commented Aug 29, 2018

Uh oh!

ferdinando-ferreira commented Aug 29, 2018

Uh oh!

ferdinando-ferreira commented Aug 30, 2018

Uh oh!

ferdinando-ferreira commented Sep 3, 2018

Uh oh!

michael-ciniawsky commented Sep 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

feat(middleware): add `content` option (`options.content.filter`, `options.content.transform`) #336

feat(middleware): add `content` option (`options.content.filter`, `options.content.transform`) #336

michael-ciniawsky Aug 29, 2018 •

edited

Loading

michael-ciniawsky Aug 29, 2018 •

edited

Loading

michael-ciniawsky Aug 29, 2018 •

edited

Loading

michael-ciniawsky Aug 29, 2018 •

edited

Loading

michael-ciniawsky Aug 29, 2018 •

edited

Loading

michael-ciniawsky commented Aug 29, 2018 •

edited

Loading

codecov bot commented Aug 29, 2018 •

edited

Loading

alexander-akait commented Aug 29, 2018 •

edited

Loading

ferdinando-ferreira commented Aug 29, 2018 •

edited

Loading

michael-ciniawsky commented Sep 3, 2018 •

edited

Loading