upgrade webpack

[timdeschryver]

Closes #1399

[nknapp]

Hi and thanks for the PR.

I noticed that the node 5 build is failing due to incompatible peer dependencies (babel-loader). My local node@8/npm@5 fails as well...

I pushed 2 commits to your branch. It seems that the babel packages need to be upgraded to version 6 and 7 in order to work with webpack 3, so I rebased my babel6-branch on your PR-branch.

There are still some problems/questions

• is the babel-option: "loose: ['es6.modules'] still somehow necessary? It does not seem to have an equivalent in babel 6
• same for the the option "optional: runtime". It does not have an equivalent, or does it?

The tests still fail: Changing import * as Handlebars from './handlebars' to import Handlebars from './handlebars' in "lib/precompiler.js" seems to fix one problem, but I am worried that this might be an incompatible change, because other libraries that use this kind of import may get problems as well.
It feels like this should not be a problem, because it is only the local webpack-babel-configuration that causes it, but I'm not completely sure.

[timdeschryver]

Sorry, was a bit busy so I couldn't fix the build on lower versions of node.
It seems to me that the options loose and optional were both removed in babel 6 - changelog

[timdeschryver]

It is strange that some tests are failing... (I have taken a quick look and don't know why tho 😕)
I do think that import Handlebars from './handlebars' would be preferable? Because the default export is Handlebars.

I'll try to take a look this week.

[timdeschryver]

Hmmm, I can't seem to fix it without breaking some tests.
Babel changed some things with the default export in babel 6? (see here).

[nknapp]

I see. There are two ways to get around this.

1. Identify all places where an "import * as foo from './bar.js'" is done inside the "lib/handlebars"-code and replace them with "import foo from './bar.js'". This also means changing the test-code in (spec/node.js) and it also means the the interface will be broken (because now you have to do const Handlebars = require('handlebars').default in node, rather than require('Handlebars').
2. Identify all places where "export default" is done, and check if it actually needs to be `export { create, registerHelper, parse ... }.

We don't want to break the interface, so option to would be preferable, but 'm not sure that this really works and if this would not break anything too.

[nknapp]

Those things I write here, currently feel like I'm given tips to you (@tdeschryver) with the expectation that you solve that problem. That is not my desire. It's great if you continue working on this. But since this is probably getting bigger than initially expected, I would also like to give more people the opportunity to work on this and try things out.

I have pushed a branch 4.x-babel6 to allow others to place PRs against this branch with possible solutions.

[timdeschryver]

Oh sorry, didn't see your messages.
I think I solved it but in a hacky way (because I'm not 100% sure how to export it properly)... 🤔

[nknapp]

The missing coverage seems to be in functions generated by babel (That's why travis is failing)

The solution is tempting, but I don't know if tools that use ES6-import natively (like rollup) will work with this fix (see #1279). This is not covered by tests, yet.

[nknapp]

Mabye a combination will work. Using "export default" and "module.exports ="

[timdeschryver]

The combination doesn't fix the code coverage.
If this is still open I'll take a another look some days later.

[timdeschryver]

OK if I let this one open for someone else? It seems like I'm overlooking something.
On a side note, I think the upgrade of webpack alone should be OK? Build errors seems to be related to npm install? If I remember correctly, I could build it locally on npm 4.

[nknapp]

Sure, leave it open. Thanks for your efforts so far.

[nknapp]

I've added the help-wanted badge. Maybe this will draw attention by others...

[nknapp]

@tdeschryver I have rebased your branch onto the current 4.x and force-pushed it. I hope that does not mess up your history.

[nknapp]

closing for now. We will have other attempts some time.