templateAdapter: avoid dynamic require if possible

[benjie]

I thought I'd experiment with packaging Rendr under webpack. I've got around most issues with some method overriding but this one remains and a monkey-patch for it seems to be very heavy-handed. I believe the patch is clean enough to not break backwards compatibility. Note templateAdapterModule is now the module itself rather than its name.

(In my code I'm overriding methods like ModelUtils::fetchConstructor, Router::getRouteBuilder, Router::getAction, and BaseView.getView to do requires from my code rather than from Rendr's context.)

[coveralls]

Coverage decreased (-0.03%) when pulling adc7f60 on benjie:require-workaround into 3914e0c on rendrjs:master.

[saponifi3d]

Hey this is pretty interesting, can you post in the issues how you're trying to package this using webpack?

Also, can you please add some tests for the code you added? Looks great overall, I like the simplicity of wrapping it in a function that can be overridden.

[benjie]

Webpack has been working great for us since I opened this issue; and it also results in a 10% smaller bundle than browserify does - plus I feel I have a lot more control over it (admittedly after having to climb a pretty steep learning curve and monkey patching Rendr in a couple of places).

Trimmed down webpack.config.js:

var webpack = require('webpack');
module.exports = {
  target: "web",
  context: __dirname,
  entry: {
    mergedAssets: "./webpack-entry.js"
  },
  module: {
    loaders: [
      {test: /console-polyfill/, loader: "imports?this=>window"},
      {test: /\.coffee$/, loader: "coffee"},
      {test: /\.json$/, loader: "json"}
    ]
  },
  resolve: {
    extensions: ["", ".coffee", ".js", ".json"],
    alias: {
      jquery$: __dirname + "/node_modules/jquery", //Zepto?
      underscore$: __dirname + "/node_modules/underscore", //lodash?
      backbone$: __dirname + "/node_modules/rendr/node_modules/backbone",
      async$: __dirname + "/node_modules/async",
      handlebars$: __dirname + "/handlebars-runtime.js", // We only need the runtime!
    }
  },
  plugins: [
    new webpack.IgnorePlugin(/^fs$/),
    new webpack.IgnorePlugin(/\.(hbs|md)$/),
    new webpack.IgnorePlugin(/\/\.[^\.]/),
    new webpack.IgnorePlugin(/\/test\//),

    // Select specific moment locales
    new webpack.ContextReplacementPlugin(/moment\/locale$/, /\/en/),

    // Fix Rendr's naughty requires
    new webpack.NormalModuleReplacementPlugin(/^app\/router$/, "../../../app/router.coffee"),
    new webpack.NormalModuleReplacementPlugin(/^app\/routes$/, "../../../app/routes.coffee"),
  ],
  output: {
    path: __dirname + "/public",
    filename: "[name].js",
    publicPath: "mergedAssets.js"
  }
}

webpack-entry.js:

window.MyApp = require('./app/app')

__layout.hbs:

<!-- all the normal stuff -->
    <script>
      var App = window.App = new MyApp({{json appData}});
      App.bootstrapData({{json bootstrappedData}});
      App.start();
    </script>
<!-- all the normal stuff -->

[saponifi3d]

After digging a bit more into webpack (so i can understand this stuffs) I'm not sure how this change is needed for webpack? is it simply for fixing the require when running with node? i'm curious if we could use something like https://github.com/webpack/enhanced-require instead to solve the problem w/o having to change this code around.

[benjie]

Nope, we don't use webpack when running in Node, only for the browser assets. This is required because without it webpack sees require(templateAdapterModule) which is a dynamic require so it automatically caches the local (to rendr) node_modules, not including the one I want (which is local to my app instead). This patch allows replacing it with an explicit static require in your own code.

There may well be a better way of doing it, I'm a webpack newbie.

[saponifi3d]

heh - me too! i'll try poking around with using enhanced-require to grab it - i think it mostly just makes require place nicely across the board. (or if you wouldn't mind giving it a shot that'd be handy too.)

[saponifi3d]

Hmm rather than passing in a string maybe we should pass an instance into the app creation for this instead? checkout: https://github.com/rendrjs/rendr/pull/447/files for an example

[pjanuario]

@benjie and @saponifi3d it seems to me that dynamic requires and alias have been one of the major problems I have read about while improving/changing/upgrading rendr bundling.

I wasn't able to create a browserify bundle with last browserify release due to alias mostly.

[alexindigo]

@pjanuario +1, instead of crufting complex requiring logic, we can just provide simple mechanism of dependency injection. Instead of calling getTemplateAdapterModule with module name, users (of the framework, i.e. developers) can pass reference to the template engine instance. That way they can manage loading of the proper components in convenient for them way.
And it will work better with AMD as well.

[pjanuario]

@alexindigo 👍

[alexindigo]

@benjie @pjanuario Now it's my turn to convert our huge Rendr app to webpack :) And it ain't easy task.
Any tips/gotchas?

I'm seeing similar warnings for other files:

WARNING in ./~/rendr/shared/base/view.js
Critical dependencies:
453:11-28 the request of a dependency is an expression
448:6-38 the request of a dependency is an expression
 @ ./~/rendr/shared/base/view.js 453:11-28 448:6-38

WARNING in ./~/rendr/shared/syncer.js
Critical dependencies:
25:11-33 the request of a dependency is an expression
 @ ./~/rendr/shared/syncer.js 25:11-33

WARNING in ./~/rendr/shared/base/router.js
Critical dependencies:
81:11-34 the request of a dependency is an expression
110:11-45 the request of a dependency is an expression
 @ ./~/rendr/shared/base/router.js 81:11-34 110:11-45

WARNING in ./~/rendr/shared/base/router.js
Critical dependencies:
205:15-22 require function is used in a way in which dependencies cannot be statically extracted
 @ ./~/rendr/shared/base/router.js 205:15-22

WARNING in ./~/rendr/shared/app.js
Critical dependencies:
97:25-91 the request of a dependency is an expression
100:29-59 the request of a dependency is an expression
 @ ./~/rendr/shared/app.js 97:25-91 100:29-59

WARNING in ./~/rendr/shared/modelUtils.js
Critical dependencies:
81:32-39 require function is used in a way in which dependencies cannot be statically extracted
83:35-42 require function is used in a way in which dependencies cannot be statically extracted
 @ ./~/rendr/shared/modelUtils.js 81:32-39 83:35-42

[saponifi3d]

@alexindigo haven't built it in webpack, but I have gotten it working fine in browserify - should be the same thing. Generally, it should just work in either environment. I'd recommend building the example app with webpack to get the config correct, then fix errors as they show up in your app.

[alexindigo]

@saponifi3d Thanks. This is pretty much what I'm doing, small surface to big surface.
But warnings show up right away.

From webpack we need on-demand loading, which is not as straightforward in browserify, and our desktop site switched to webpack already, so we don't have much say here. :)

[pjanuario]

@saponifi3d @alexindigo i spent some quite time trying to update to lastest versions of browserify and the main reason why i put it on hold was related with the dynamic requires and alias since I mentioned before. I think @alexindigo suggestion to use dependency injection might be the best approach, but for that we need to change rendr core. Currently I am not spending that much time with rendr at the moment.

[alexindigo]

@pjanuario thanks for the update. Yep, I'm in tight deadline at the moment, so will be hacking things in-place before submitting changes to upstream.

[benjie]

Sorry I can't be much help; we moved off of rendr to react+react-router a long time ago (building our own rendr compatibility layers in the process to ease the transition - probably not that useful to others, alas, but if I recall correctly it only took about 3 days and a whole load of Vim macros despite the very large size of our codebase!).

The warnings you see above are relatively harmless; it's probably from things like rendr requiring "app/models/" + modelName (i.e. dynamic requires); webpack tends to handle these just fine by pulling in everything from that directory so it can be dynamically required at run time - the only major issue with that is that you sometimes pull in more code than is necessary into your bundle.

If you're interested I highly recommend using React in your Rendr stack as the rendering engine (rather than Backbone views); we did a presentation on it a couple years ago:

http://timecounts.github.io/backbone-react-redux/#1

(Since then we've moved away from CoffeeScript to ES6 because CoffeeScript refuses to implement things like object splats and because ESLint is awesome compared to any of the offerings from the CoffeeScript community. We've also started implemented the "proper" way of doing React - i.e. using Flux instead of backbone models; this is a slow transition though so we've again got compatibility layers and use both flux and backbone in parallel in many places.)

[alexindigo]

@benjie This is where are heading as well. Webpack being the first step.
Thank you for suggestions and for the link.

PS. I don't have much vim-macros fu, I'm more awk kind of guy, so we'll see. :)

[benjie]

Okay, so this will be my last OT comment but I feel it's worth mentioning we've recently switched from webpack back to browserify because HMR on webpack was being too slow (due to this bug: webpack/webpack#1530). Since then Webpack have released 2.0 so maybe it's sorted now? My advice is to keep away from webpack-specific loaders (like style-loader) so that you can easily switch between webpack/browserify/rollup/whatever rather than being tied down.

[alexindigo]

Cool thanks. I'll try to keep webpack for JS only things.
Webpack2 is still in beta btw.

PS. We really need on-demand loading and I couldn't figure it out with browserify.