webpack: Use handlebars-loader to handle frontend templates.

[tommyip]

And remove the compile-handlebars-templates system.

[tommyip]

We can do this or add the path to all the references to settings/widgets.

[andersk]

Whoa. I didn’t realize webpack has special support for require() on computed strings like that, but yes, apparently it’s smart enough to parse that computation, find all the *.handlebars files within static/templates, compile them, and bundle them into a map for runtime dispatch. Wouldn’t have been my first plan but I guess it works!

We don’t want to catch exceptions from rendering here, so the (arg) should be lifted outside the try…catch. And using require.context might help webpack generate slightly more efficient code:

var template_context = require.context('../templates', true, /\.handlebars$/);
var template_paths = ['./', './settings/', './widgets/'];

exports.render = function (name, arg) {
    for (var i = 0; i < template_paths.length; i += 1) {
        var template;
        try {
            template = template_context(template_paths[i] + name + '.handlebars');
        } catch (_e) {
            continue;
        }
        return template(arg);
    }
    throw new Error('Cannot find template ' + name + '.handlebars anywhere '
        + 'under the static/templates/ folder.');
};

(Edit: require.context doesn’t work in the Node tests, so skip that part.)

I think I’d be in favor of adding explicit paths for settings and widgets so as to reduce the amount of magic here.

[timabbott]

Yeah, the only reason we didn't have explicit paths for rendering settings/foo.handlebars was because the old system didn't support naming things that way. So I think passing e.g. settings/foo.handlebars into .render is the right approach over doing a loop here.

[timabbott]

What's the story with this change? Are those two both files in the module, or what?

[tommyip]

When importing the module without explicitly specifying the path, ie import "handlebars/runtime";, webpack actually resolve it to handlebars/dist/cjs/handlebars.runtime.js. Also after adding handlebars-loader, the original Handlebars from handlebars/dist/handlebars.runtime.js can't find the templates anymore for some reason.

Also is there a reason why we specify the exact file we import instead of just giving the name?

[andersk]

In some cases, it makes a difference because whatever/dist/whatever.js adds itself to the global window namespace while whatever is a module that expects to be used the way modules are supposed to be used. We have lots of code that pulls things from window but we should migrate to the module style whenever it’s convenient. In this case, go with handlebars/runtime if you can; that’s the documented way.

[timabbott]

Yeah, the window hack is a mess but solved the problem of being able to migrate most asset stuff into webpack. If we can do import handlebars/runtime, that'd be great.

[timabbott]

What's the story with this list? Does it need to be synchronized with some list elsewhere in the codebase?

If so we should add comments in both places making clear the need to sync the lists, and I'd be curious to know what the exception is if we do it wrong.

[tommyip]

These are the helpers defined in templates.js combined with the ones we originally passed to handlebars in compile-handlebar-templates line 33. This is to let webpack know these helpers are defined at run time and don't try to require them when bundling. Nothing will happen if these list is not correct, handlebar-loader just have to do more guessing work.

I'll add some comments.

[timabbott]

OK.

[tommyip]

Removed the loop and lifted the (arg).

[timabbott]

Awesome, I'm going to test-deploy this on chat.zulip.org and merge it if it works, since the remaining comments seem like things we can do as follow-ups.

[timabbott]

Seems to work in production, so merged, thanks @tommyip! I do think it's still worth looking at whether we can do something nicer for our third-party packages with #12667 (comment).

[tommyip]

Yeah, the window hack is a mess but solved the problem of being able to migrate most asset stuff into webpack. If we can do import handlebars/runtime, that'd be great.

it makes a difference because whatever/dist/whatever.js adds itself to the global window namespace while whatever is a module that expects to be used the way modules are supposed to be used

We already use webpack expose-loader to attach a lot of our libraries to window, might as well be consistent and do that for everything and use the normal importing style. (And overtime do the importing inside individual modules as part of the typescript migration)

[andersk]

No, see, this isn’t about consistency vs. inconsistency, it’s about right vs. wrong. Attaching everything to window wasn’t a stylistic choice. It was a necessary transitional step on the path from where Zulip started, with no Webpack or bundler of any kind except the one where you strcat all your libraries together, to where it should be. It’s an antipattern that isn’t type-safe and weakens the ability for both humans and tools to reason about your code. You can configure TypeScript and ESLint to expect that window has been altered, but there’s no static guarantee that your configuration matches reality. Webpack can’t see through this pattern so optimizations like tree shaking are inhibited. IDEs can’t show you their fancy auto-complete menus. Plus it’s just not what modern JS developers do. I’m not saying we need to drop everything and fix it all right now, but we shouldn’t use “consistency” as a justification for perpetuating it.

[tommyip]

Yeah, that's why I said

overtime do the importing inside individual modules as part of the typescript migration

which I am already in the process of doing (as well as other stuff to give webpack and editors better information)

What I mean by consistency is the way we configure our third party library is inconsistent. Some library such as moment is attached to window as a side effect (import "moment/min/moment.min.js";) while some uses expose-loader, which is confusing.