feat(middleware): add `generateContent` option (`options.generateContent`) #330

ferdinando-ferreira · 2018-08-22T19:14:10Z

What kind of change does this PR introduce?
Feature

Did you add tests for your changes?
Yes

Summary
This change adds a new option to the middleware: generateContent, to allow for the middleware, optionally, to accept a function that is capable of modifying the served files on the fly.

Does this PR introduce a breaking change?
No, given that the new option is is optional and that its default value results in the same previously existing behaviour there won't be a breaking change.

Other information
The impact on the middleware itself is very limited, the purpose of this feature would be to allow for (let's say) webpack-serve, using the serve.devMiddleware option, to set this option and allow for the middleware to change files on the fly (for instance, adding a script to the html, adding a watermark to served images) or to signal to the middleware that it must not serve that file. Below is an excerpt of a webpack.config.js using this option (also featured in the tests and the documentation sample in a more simplified fashion).

serve: {
  devMiddleware: {
    publicPath: this.getOutput(target).publicPath,
    methods: ['GET', 'POST'],
    generateContent: (context, filename, req, res, defaultGenerator) => {
      const serialize = require('serialize-javascript');
      let content = defaultGenerator(context, filename, req, res);
      if (/\.html$/.test(filename)) {
        const search = '<!--self generated vars-->';
        let content_string = content.toString();
        if ((new RegExp(search)).test(content_string)) {
          let obj = {
            method: req.method
          };
          let replace = `<script type="text/javascript">var global_context = ${serialize(obj)};</script>`;
          let replaced = content_string.replace(search, replace);
          content = Buffer.from(replaced);
        }
      }
      return content;
    }
  }
}

The effect is to transform the following .html (served in a typical vue SPA)

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="utf-8">
    <title>plataforma</title>
    <meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1">
    <meta name="mobile-web-app-capable" content="yes">
    <meta name="apple-mobile-web-app-capable" content="yes">
  </head>
  <body>
    <div id="app"></div>
    <!--self generated vars-->
    <script type="text/javascript" src="/index.js"></script>
  </body>
</html>

into this

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="utf-8">
    <title>plataforma</title>
    <meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1">
    <meta name="mobile-web-app-capable" content="yes">
    <meta name="apple-mobile-web-app-capable" content="yes">
  </head>
  <body>
    <div id="app"></div>
    <script type="text/javascript">var global_context = {"method":"GET"};</script>
    <script type="text/javascript" src="/index.js"></script>
  </body>
</html>

Best regards

michael-ciniawsky

Sry but I'm not in favor of making this change, either adding another middleware or handle it within the webpack build itself should be sufficient for most of these use cases. For this to be considered landing, concrete use cases which aren't doable otherwise need to be named first

Convince me though :). Definitely open for discussion

michael-ciniawsky · 2018-08-22T20:51:39Z

README.md

+
+This option accepts a `Function` value which is called before the files being served are read from the filesystem. It can be used to modify the file on the fly, skip their processing altogether or simply log their generation. For instance, the following configuration will replace the string `<!--self generated vars-->` with `<script type="text/javascript">var global_request = { method: "GET" }</script>` in every .html file requested via HTTP GET.
+
+```js


This example is definitely too complicated as an description :)

Heh, I agree, I used the actual use case I'm proposing this function tho. It could be changed to the one used in the test suite (a simpler replace).

Here is the real world use case this would solve (and the whole motivation for this PR): as of today here is no way to detect if the request was a POST and, now that POSTs are accepted at all (due to the methods recently accepted option), there is no way to actually pass the post data to the scripts.

The example contained in the readme does just that, but partially. Parse the POST body and add to the global_request var and it becomes possible to inform the scripts what the actual posted data was.

That's an overkill for the README, I agree, should I change it to the "My" -> "Modified" example I included in the tests?

michael-ciniawsky · 2018-08-22T20:53:39Z

lib/middleware.js

@@ -65,7 +65,12 @@ module.exports = function wrapper(context) {
        }

        // server content
-        let content = context.fs.readFileSync(filename);
+        const defaultGenerator = ((context, filename, req, res) => context.fs.readFileSync(filename));


Why passing res, req as params ? (Use Cases)

It is the signature of the function, the parameters that today are currently available to generate the content.

It makes no difference today but, in the future, they may be required for the base middleware to perform some filtering, some operation (not foreseeable today) and adding them back to the function would be a breaking change. Basically it is a case of the child function passing back what they received in the first place, to make it easier to chain.

ferdinando-ferreira · 2018-08-22T22:25:11Z

either adding another middleware or handle it within the webpack build itself should be sufficient for most of these use cases (...) Convince me though :). Definitely open for discussion

Here is the real use case: as of now there is no way to perform a series of tasks on webpack-serve, a customer of webpack-dev-middleware, particularly changing the index.html on pageview instead of on compilation.

Even an otherwise simple task like having a <title> tag change based on which page is loaded, is (to the best of my knowledge) not possible. Adding a middleware down the line (with app.use on the add option) doesn't actually solve the problem because devMiddleware handles all the memory based filesystem requests and the webpack context is not exposed in any way through that method.

This change is backwards compatible, simple enough (4 lines) to have its consequences fully understandable and opens possibilities not implementable by any other means. Here is a partial list of tasks this would allow that (again, to the best of my knowledge, would not be possible)

1, Inject meta tags to the index.html generated by html-webpack-plugin (like title or meta description)
2, add watermarks to images generated on the fly
3, instruct this middleware not to generate certain filed so other middlewares down the line can do it instead

Essentially this change fulfills the idea of the Server-Side Rendering option mentioned in the docs (which I assume are going away based on the Project notes) in a cleaner, more generic and more powerful manner and with no drawback (that I can imagine)

michael-ciniawsky · 2018-08-22T23:48:52Z

Inject meta tags to the index.html generated by html-webpack-plugin (like title or meta description)

For different (additional) meta tags based on the particular request, right ? Otherwise this should by solved by the html-webpack-plugin already. Please elaborate here

add watermarks to images generated on the fly

I don't know of a webpack plugin which does that, but I think it is possible to solve via a plugin (but could be wrong here). Please provide an example (if possible) here

instruct this middleware not to generate certain filed so other middlewares down the line can do it instead

Ok. But this needs design e.g a options.content option => options.content.filter(fn) (3) + eventually options.content.transform(fn) ([1, 2]) (currently proposed as generateContent) as it sounds like 3 is a different use case then 'generating' content :), where transform {Function} is more like just e.g (url, content) => something(content)

Essentially this change fulfills the idea of the Server-Side Rendering option mentioned in the docs (which I assume are going away based on the Project notes) in a cleaner, more generic and more powerful manner and with no drawback (that I can imagine)

That sounds interesting, an idea for a more generic approach is appreciated here. In terms of removal nothing is set in stone yet, but the term server side rendering is confusing imho and I would like to either refactor and rename it (no concrete plans) or simply remove it. But this needs consensus, I'm voicing my personally here

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

That sounds interesting, an idea for a more generic approach is appreciated here.

I'll write a detailed explanation later but, for now, I prepared an example of what can be achieved with this potential new option.

Here are the install instruction for this sample, it is a fully featured turnkey hot reloaded Server Side rendered single file component powered environment to develop vuejs applications.

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

This little environment allows for development of Vue applications without actually having to restart the server, recompile manually or even generate the files on disk, it all happens in memory. It may not be a fully drop in replacement for nuxt but it is close enough to be useful.

I plan to write the second (unrelated, for the purpose of this exercise) half of this and write the production side of the environment to fulfill the potential of the tool but, for now, this illustrates the potential of this new option with regards to the possibilities to extend webpack-serve through webpack-dev-middleware.

Full write up later, please install and take a look.

michael-ciniawsky · 2018-08-24T16:55:03Z

I will take a look at the example as soon as I have the time, but in general

vuejs applications

a particular framework's limitations or concerns of course influence decisions made for webpack-dev-middleware but it's definitely not sufficient to justify adding a new option. I understand this might be just used as an example here, but we need to triage the underlying concepts and find a 'as possible' generic design for the option/API independent from any framework design by default. I'm not happy with the name and design of this option as is. It smells like 'quick and dirty' ad-hoc solution atm (no offense, or being agressive here, it's fine to do that for personal usage). Every new option needs to be maintained and will likely stay for a while and potential usage of this particuar addition is also still vague and broader in scope. We need to make sure to identify the potential use case, potential issues/misuses (in conjunction with other options, webpack-dev-server etc etc) and how this may replace options.serverSideRender/improve SSR in particular in future and iterate

ferdinando-ferreira · 2018-08-24T17:06:29Z

no offense

None taken

We need to make sure to identify the potential use case, potential issues/misuses (in conjunction with other options, webpack-dev-server etc etc) and how this may replace options.serverSideRender/improve SSR in particular in future and iterate

Duly noted

But we need to triage the underlying concepts and find a 'as possible' generic design for the option/API . I'm not happy with the name and design of this option as is. It smells like 'quick and dirty' ad-hoc solution atm

Your assessment is correct (it is mostly brainstorming in public up until now), one of the reasons I created a separated branch instead of polluting the Pull Request even more.

I understand this might be just used as an example here (...) independent from any framework design by default

The vue is just an example, there is nothing specific in that particular implementation except the fact a real world example was used to illustrate the usefulness.

Test the sample provided if you can and I'll write a full write-up, think on a non provisional naming the option and try to make the case for it's inclusion.

michael-ciniawsky · 2018-08-24T17:51:11Z

The vue is just an example, there is nothing specific in that particular implementation except the fact a real world example was used to illustrate the usefulness.

Yep, understood and ssumed since generateContent is generic here already

Test the sample provided if you can and I'll write a full write-up, think on a non provisional naming the option and try to make the case for it's inclusion.

The particular name is the least important for now, sothing lik either options.content or if more SSR related options.ssr as an {Object} with properties for each needed area e.g

app.use(middleware(compiler, {
   content: {
      filter (url) {},
      transform (content) {},
      generate (req, res, fs) {},
      ...,
  }
}))

ferdinando-ferreira · 2018-08-28T16:56:57Z

Closing this PR so I can submit the another one, synced with the current master and properly implemented

ferdinando-ferreira requested review from alexander-akait and michael-ciniawsky as code owners August 22, 2018 19:14

michael-ciniawsky changed the title ~~New option: generateContent~~ feat(middleware): add generateContent option (options.generateContent) Aug 22, 2018

michael-ciniawsky suggested changes Aug 22, 2018

View reviewed changes

michael-ciniawsky added this to the 3.2.0 milestone Aug 22, 2018

michael-ciniawsky added semver: minor type: feature pr: review status: discuss labels Aug 22, 2018

michael-ciniawsky mentioned this pull request Aug 23, 2018

Server Side Rendering + Express #306

Closed

2 tasks

michael-ciniawsky force-pushed the master branch 2 times, most recently from 908dd4f to 38077ef Compare August 23, 2018 20:00

ferdinando-ferreira closed this Aug 28, 2018

michael-ciniawsky added status: declined and removed pr: review status: discuss labels Aug 28, 2018

michael-ciniawsky removed this from the 3.3.0 milestone Aug 28, 2018

ferdinando-ferreira mentioned this pull request Aug 29, 2018

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

feat(middleware): add `generateContent` option (`options.generateContent`) #330

feat(middleware): add `generateContent` option (`options.generateContent`) #330

Uh oh!

ferdinando-ferreira commented Aug 22, 2018

Uh oh!

michael-ciniawsky left a comment •

edited

Loading

Uh oh!

michael-ciniawsky Aug 22, 2018 •

edited

Loading

Uh oh!

ferdinando-ferreira Aug 22, 2018

Uh oh!

michael-ciniawsky Aug 22, 2018

Uh oh!

ferdinando-ferreira Aug 22, 2018

Uh oh!

ferdinando-ferreira commented Aug 22, 2018 •

edited

Loading

Uh oh!

michael-ciniawsky commented Aug 22, 2018 •

edited

Loading

Uh oh!

ferdinando-ferreira commented Aug 24, 2018

Uh oh!

michael-ciniawsky commented Aug 24, 2018 •

edited

Loading

Uh oh!

ferdinando-ferreira commented Aug 24, 2018 •

edited

Loading

Uh oh!

michael-ciniawsky commented Aug 24, 2018

Uh oh!

ferdinando-ferreira commented Aug 28, 2018

Uh oh!

Uh oh!


		This option accepts a `Function` value which is called before the files being served are read from the filesystem. It can be used to modify the file on the fly, skip their processing altogether or simply log their generation. For instance, the following configuration will replace the string `<!--self generated vars-->` with `<script type="text/javascript">var global_request = { method: "GET" }</script>` in every .html file requested via HTTP GET.

		```js

Uh oh!

feat(middleware): add generateContent option (options.generateContent) #330

feat(middleware): add generateContent option (options.generateContent) #330

Uh oh!

Conversation

ferdinando-ferreira commented Aug 22, 2018

Uh oh!

michael-ciniawsky left a comment • 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 22, 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!

ferdinando-ferreira Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

ferdinando-ferreira Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ferdinando-ferreira commented Aug 24, 2018

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

michael-ciniawsky commented Aug 24, 2018

Uh oh!

ferdinando-ferreira commented Aug 28, 2018

Uh oh!

Uh oh!

feat(middleware): add `generateContent` option (`options.generateContent`) #330

feat(middleware): add `generateContent` option (`options.generateContent`) #330

michael-ciniawsky left a comment •

edited

Loading

michael-ciniawsky Aug 22, 2018 •

edited

Loading

ferdinando-ferreira commented Aug 22, 2018 •

edited

Loading

michael-ciniawsky commented Aug 22, 2018 •

edited

Loading

michael-ciniawsky commented Aug 24, 2018 •

edited

Loading

ferdinando-ferreira commented Aug 24, 2018 •

edited

Loading