New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

webpack 4.0.0-alpha.3/4 feedback #6244

Closed
sokra opened this Issue Jan 4, 2018 · 44 comments

Comments

Projects
None yet
@sokra
Member

sokra commented Jan 4, 2018

4.0.0-alpha.0
4.0.0-alpha.1
4.0.0-alpha.2

Features

  • allow to import JSON via ESM syntax
  • add unused exports elimination for JSON modules
  • many config options which support placeholders now also support a function
  • entry defaults to ./src
  • output.path defaults to ./dist
  • add output.globalObject config option to allow to choose the global object reference in runtime exitCode
  • timestamps for files are read from watcher when calling Watching.invalidate
  • Use production defaults when omiting the mode option
  • add this.hot flag to loader context when HMR is enabled
  • Add detailed progress reporting to SourceMapDevToolPlugin

Bugfixes

  • Avoid using process.exit and use process.exitCode instead
  • fix bluebird warning in webpack/hot/signal
  • fix incorrect -! behavior with post loaders
  • add run and watchRun hooks for MultiCompiler
  • (.4) fix json modules in concatenated modules
  • (.4) remove extra semi

Internal changes

  • Migrated many deprecated plugins to new plugin system API
  • buildMeta.harmony has been replaced with buildMeta.exportsType: "namespace
  • added buildMeta.exportsType: "default" for json modules
  • Remove unused methods from Parser (parserStringArray, parserCalculatedStringArray)
  • Remove ability to clear BasicEvaluatedExpression and to have multiple types
  • (.4) performance improvement for RemoveParentModulesPlugin

Please comment if you find additional changes not in the changelog

@KidkArolis

This comment has been minimized.

Show comment
Hide comment
@KidkArolis

KidkArolis Jan 4, 2018

Hm.. why not . as default entry? Similar to node .. Means you can create index.js and choose your src dir. (It's what I'm exploring in https://github.com/KidkArolis/jetpack)

KidkArolis commented Jan 4, 2018

Hm.. why not . as default entry? Similar to node .. Means you can create index.js and choose your src dir. (It's what I'm exploring in https://github.com/KidkArolis/jetpack)

@donaldpipowitch

This comment has been minimized.

Show comment
Hide comment
@donaldpipowitch

donaldpipowitch Jan 5, 2018

I like ./src as a default entry. I personally see more projects with a src folder than anything else (maybe someone could analyze projects on GitHub for this...?) and there is probably less need to have a different directory name (so why not offer a default name). As a nice side effect: other languages/frameworks also use ./src quite often (e.g. Rust with cargo).

donaldpipowitch commented Jan 5, 2018

I like ./src as a default entry. I personally see more projects with a src folder than anything else (maybe someone could analyze projects on GitHub for this...?) and there is probably less need to have a different directory name (so why not offer a default name). As a nice side effect: other languages/frameworks also use ./src quite often (e.g. Rust with cargo).

@sompylasar

This comment has been minimized.

Show comment
Hide comment
@sompylasar

sompylasar Jan 5, 2018

I agree with @donaldpipowitch, and having src separated from everything else is a pattern that scales better. Leave the root for directories and metadata (package.json etc.).

sompylasar commented Jan 5, 2018

I agree with @donaldpipowitch, and having src separated from everything else is a pattern that scales better. Leave the root for directories and metadata (package.json etc.).

@TheLarkInn

This comment has been minimized.

Show comment
Hide comment
@TheLarkInn

TheLarkInn Jan 5, 2018

Member

Precisely what @donaldpipowitch said. This is our opportunity to use a pretty common pattern as a default.

Member

TheLarkInn commented Jan 5, 2018

Precisely what @donaldpipowitch said. This is our opportunity to use a pretty common pattern as a default.

@probablyup

This comment has been minimized.

Show comment
Hide comment
@probablyup

probablyup Jan 5, 2018

Contributor

I would question not inspecting the package.json main field and using that as the entry.

Contributor

probablyup commented Jan 5, 2018

I would question not inspecting the package.json main field and using that as the entry.

@phyllisstein

This comment has been minimized.

Show comment
Hide comment
@phyllisstein

phyllisstein Jan 6, 2018

Just following up on #6179 (comment): there was indeed a second Webpack instance installed, by webpack-common-shake; removing it resolved the issue.

Now I'm seeing some issues with JSON in production. Without using a JSON loader, the file attaches itself to module.exports...

  urHsdkTj: function(e) {
    e.exports = {
      white: "#ffffff",
      black: "#000000",

...but when I import it the code looks for exports.default:

      , Z = n("urHsdkTj")
      , Q = K()(Z.default.gray[9]).alpha(.2).css()

Doesn't appear to be an issue in development mode, where .default isn't added to the import:

var UMBRA_COLOR = chroma_js__WEBPACK_IMPORTED_MODULE_3___default()(open_color_open_color_json__WEBPACK_IMPORTED_MODULE_4__.gray[9]).alpha(0.2).css();

phyllisstein commented Jan 6, 2018

Just following up on #6179 (comment): there was indeed a second Webpack instance installed, by webpack-common-shake; removing it resolved the issue.

Now I'm seeing some issues with JSON in production. Without using a JSON loader, the file attaches itself to module.exports...

  urHsdkTj: function(e) {
    e.exports = {
      white: "#ffffff",
      black: "#000000",

...but when I import it the code looks for exports.default:

      , Z = n("urHsdkTj")
      , Q = K()(Z.default.gray[9]).alpha(.2).css()

Doesn't appear to be an issue in development mode, where .default isn't added to the import:

var UMBRA_COLOR = chroma_js__WEBPACK_IMPORTED_MODULE_3___default()(open_color_open_color_json__WEBPACK_IMPORTED_MODULE_4__.gray[9]).alpha(0.2).css();

@sokra sokra changed the title from webpack 4.0.0-alpha.3 feedback to webpack 4.0.0-alpha.3/4 feedback Jan 6, 2018

@sokra

This comment has been minimized.

Show comment
Hide comment
@sokra

sokra Jan 6, 2018

Member

@phyllisstein There is a bugfix for JSON modules in concatenated modules in 4.0.0-alpha.4. This probably fixes your issue.

Member

sokra commented Jan 6, 2018

@phyllisstein There is a bugfix for JSON modules in concatenated modules in 4.0.0-alpha.4. This probably fixes your issue.

@ooflorent

This comment has been minimized.

Show comment
Hide comment
@ooflorent

ooflorent Jan 6, 2018

Member

@michael-ciniawsky Does it happen since 4.0.0-alpha.3 or 4.0.0-alpha.4? Does it work as expected in 4.0.0-alpha.2?

Member

ooflorent commented Jan 6, 2018

@michael-ciniawsky Does it happen since 4.0.0-alpha.3 or 4.0.0-alpha.4? Does it work as expected in 4.0.0-alpha.2?

@webpack webpack deleted a comment from KidkArolis Jan 6, 2018

@michael-ciniawsky

This comment has been minimized.

Show comment
Hide comment
@michael-ciniawsky

michael-ciniawsky Jan 6, 2018

Member

@ooflorent Affects all webpack v4.0.0 alphas as it seems :)

The issue was on my side, all good :D

// HACK
this._module.type = 'text/css' // don't do that :)

😅 🙃

Member

michael-ciniawsky commented Jan 6, 2018

@ooflorent Affects all webpack v4.0.0 alphas as it seems :)

The issue was on my side, all good :D

// HACK
this._module.type = 'text/css' // don't do that :)

😅 🙃

@sokra

This comment has been minimized.

Show comment
Hide comment
@sokra

sokra Jan 6, 2018

Member

hmm weird. What's the configuration?

Member

sokra commented Jan 6, 2018

hmm weird. What's the configuration?

@sokra

This comment has been minimized.

Show comment
Hide comment
@sokra

sokra Jan 6, 2018

Member

yeah... don't do that...

Member

sokra commented Jan 6, 2018

yeah... don't do that...

@andkhong

This comment has been minimized.

Show comment
Hide comment
@andkhong

andkhong Jan 7, 2018

Can't wait to try out webpack 4!

andkhong commented Jan 7, 2018

Can't wait to try out webpack 4!

@phyllisstein

This comment has been minimized.

Show comment
Hide comment
@phyllisstein

phyllisstein Jan 7, 2018

@sokra Sure does! Thanks for being on top of all this, super excited for the new version.

phyllisstein commented Jan 7, 2018

@sokra Sure does! Thanks for being on top of all this, super excited for the new version.

@dvakatsiienko

This comment has been minimized.

Show comment
Hide comment
@dvakatsiienko

dvakatsiienko Jan 7, 2018

+1 on defaults! 🔮🌠

dvakatsiienko commented Jan 7, 2018

+1 on defaults! 🔮🌠

@KidkArolis

This comment has been minimized.

Show comment
Hide comment
@KidkArolis

KidkArolis Jan 7, 2018

Just to expand on my point above and to echo @probablyup's comment.

If webpack defaults to . instead of src it allows for more familiarity and flexibility imho. With . it means that:

  • if current dir has a package.json with main or browser, that main is gonna be used as the entry
  • if no package.json is found, index.js is gonna be used

This is nice because it's very similar to how node works and is therefore familiar. And it is somewhat more flexible, because then people can choose the entry dir/module while still not writing any webpack config. For example I personally tend to use app or lib more than src.

Having said all this, webpack config is still there! You can point it to whatever you need to anyway.

KidkArolis commented Jan 7, 2018

Just to expand on my point above and to echo @probablyup's comment.

If webpack defaults to . instead of src it allows for more familiarity and flexibility imho. With . it means that:

  • if current dir has a package.json with main or browser, that main is gonna be used as the entry
  • if no package.json is found, index.js is gonna be used

This is nice because it's very similar to how node works and is therefore familiar. And it is somewhat more flexible, because then people can choose the entry dir/module while still not writing any webpack config. For example I personally tend to use app or lib more than src.

Having said all this, webpack config is still there! You can point it to whatever you need to anyway.

@sompylasar

This comment has been minimized.

Show comment
Hide comment
@sompylasar

sompylasar Jan 7, 2018

@KidkArolis That's actually a good point if package.json references will be considered.

sompylasar commented Jan 7, 2018

@KidkArolis That's actually a good point if package.json references will be considered.

@KidkArolis

This comment has been minimized.

Show comment
Hide comment
@KidkArolis

KidkArolis Jan 7, 2018

That's already how webpack works, if you point entry config to a dir with package.json it will find the entry module with all the same rules as when looking for an entry to an npm package. E.g. will look at main and browser of package.json and other things it looks at.

KidkArolis commented Jan 7, 2018

That's already how webpack works, if you point entry config to a dir with package.json it will find the entry module with all the same rules as when looking for an entry to an npm package. E.g. will look at main and browser of package.json and other things it looks at.

@filipesilva

This comment has been minimized.

Show comment
Hide comment
@filipesilva

filipesilva Jan 8, 2018

Contributor

I noticed that UglifyJS is loaded automatically as part of the option defaults:

this.set("optimization.minimizer", "make", options => [{
apply: compiler => {
// Lazy load the uglifyjs plugin
const UglifyJsPlugin = require("uglifyjs-webpack-plugin");
new UglifyJsPlugin({
cache: true,
parallel: true,
sourceMap: options.devtool && /source-?map/.test(options.devtool)
}).apply(compiler);
}
}]);

What happens when users want to add a different version or different options?

Contributor

filipesilva commented Jan 8, 2018

I noticed that UglifyJS is loaded automatically as part of the option defaults:

this.set("optimization.minimizer", "make", options => [{
apply: compiler => {
// Lazy load the uglifyjs plugin
const UglifyJsPlugin = require("uglifyjs-webpack-plugin");
new UglifyJsPlugin({
cache: true,
parallel: true,
sourceMap: options.devtool && /source-?map/.test(options.devtool)
}).apply(compiler);
}
}]);

What happens when users want to add a different version or different options?

@sokra

This comment has been minimized.

Show comment
Hide comment
@sokra

sokra Jan 8, 2018

Member

@filipesilva You can just override the option:

const MinifyPlugin = require("babel-minify-webpack-plugin");
module.exports = {
  optimization: {
    minimizer: [
      new MinifyPlugin(minifyOpts, pluginOpts)
    ]
  }
}

The same way you can add additional minimizer i. e. for CSS.

Member

sokra commented Jan 8, 2018

@filipesilva You can just override the option:

const MinifyPlugin = require("babel-minify-webpack-plugin");
module.exports = {
  optimization: {
    minimizer: [
      new MinifyPlugin(minifyOpts, pluginOpts)
    ]
  }
}

The same way you can add additional minimizer i. e. for CSS.

@filipesilva

This comment has been minimized.

Show comment
Hide comment
@filipesilva

filipesilva Jan 8, 2018

Contributor

Coolio, did not know that 👍

Contributor

filipesilva commented Jan 8, 2018

Coolio, did not know that 👍

@sompylasar

This comment has been minimized.

Show comment
Hide comment
@sompylasar

sompylasar Jan 8, 2018

@sokra RE: overriding options, could you please give a few more examples? My concern is overriding an array versus extending it (adding additional minimizer). And please put the name of the file which you show a snippet from, it's never too verbose when giving examples which stay on the Internet and show up in Google results. Thank you.

sompylasar commented Jan 8, 2018

@sokra RE: overriding options, could you please give a few more examples? My concern is overriding an array versus extending it (adding additional minimizer). And please put the name of the file which you show a snippet from, it's never too verbose when giving examples which stay on the Internet and show up in Google results. Thank you.

@jchitel

This comment has been minimized.

Show comment
Hide comment
@jchitel

jchitel Jan 9, 2018

Just out of curiosity, is there a checklist, or a "definition of done" for this alpha stage? All I've seen is these feedback issues. Basically, what is required before v4 moves to beta?

No rush, I'd just like a better idea of a timeline if there is one.

jchitel commented Jan 9, 2018

Just out of curiosity, is there a checklist, or a "definition of done" for this alpha stage? All I've seen is these feedback issues. Basically, what is required before v4 moves to beta?

No rush, I'd just like a better idea of a timeline if there is one.

@errorx666

This comment has been minimized.

Show comment
Hide comment
@errorx666

errorx666 Jan 10, 2018

@probablyup I expect the package.json main to refer to the build output, not its input.

errorx666 commented Jan 10, 2018

@probablyup I expect the package.json main to refer to the build output, not its input.

@TheLarkInn

This comment has been minimized.

Show comment
Hide comment
@TheLarkInn

TheLarkInn Jan 10, 2018

Member

@jchitel we are very close for alpha => beta, but we've been trying to keep as much feature work (as possible) in alpha. This way, we can purely focus on stabilization for beta=>RC, etc

We are overdue on a list but currently its a bit nebulous at the moment. The large ticket items are complete right now and we've been focusing on UX/Defaults/Build Perf small items at the moment.

Member

TheLarkInn commented Jan 10, 2018

@jchitel we are very close for alpha => beta, but we've been trying to keep as much feature work (as possible) in alpha. This way, we can purely focus on stabilization for beta=>RC, etc

We are overdue on a list but currently its a bit nebulous at the moment. The large ticket items are complete right now and we've been focusing on UX/Defaults/Build Perf small items at the moment.

@kidwm

This comment has been minimized.

Show comment
Hide comment
@kidwm

kidwm Jan 11, 2018

@TheLarkInn How about this feature, is it feasible? #5433

kidwm commented Jan 11, 2018

@TheLarkInn How about this feature, is it feasible? #5433

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 12, 2018

from entry ./src to output.path ./dist
It's to readable default structure like angular

ghost commented Jan 12, 2018

from entry ./src to output.path ./dist
It's to readable default structure like angular

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Jan 14, 2018

Contributor

Hey all,

I'm just turning my attention to trying out ts-loader and fork-ts-checker-webpack-plugin with webpack 4. Would you be able to advise what the advised upgrade path for plugins is that currently use the compiler.applyPluginsAsync hook? eg:

      this.compiler.applyPluginsAsync('fork-ts-checker-service-before-start', () => {

Presently erroring out with:

            _this.compiler.applyPluginsAsync('fork-ts-checker-service-before-start', function () {
                           ^

TypeError: _this.compiler.applyPluginsAsync is not a function
    at C:\source\seedy-auth0\ClientApp\node_modules\fork-ts-checker-webpack-plugin\lib\index.js:143:28
    at SyncHook.eval [as call] (eval at create (C:\source\seedy-auth0\ClientApp\node_modules\tapable\lib\HookCodeFactory.js:17:12), <anonymous>:7:1)
// and on til dawn

Linking back to the migration guide you provided for compiler.plugin("before-compile", -> compiler.hooks.beforeCompile.tap

#6064 (comment)

Contributor

johnnyreilly commented Jan 14, 2018

Hey all,

I'm just turning my attention to trying out ts-loader and fork-ts-checker-webpack-plugin with webpack 4. Would you be able to advise what the advised upgrade path for plugins is that currently use the compiler.applyPluginsAsync hook? eg:

      this.compiler.applyPluginsAsync('fork-ts-checker-service-before-start', () => {

Presently erroring out with:

            _this.compiler.applyPluginsAsync('fork-ts-checker-service-before-start', function () {
                           ^

TypeError: _this.compiler.applyPluginsAsync is not a function
    at C:\source\seedy-auth0\ClientApp\node_modules\fork-ts-checker-webpack-plugin\lib\index.js:143:28
    at SyncHook.eval [as call] (eval at create (C:\source\seedy-auth0\ClientApp\node_modules\tapable\lib\HookCodeFactory.js:17:12), <anonymous>:7:1)
// and on til dawn

Linking back to the migration guide you provided for compiler.plugin("before-compile", -> compiler.hooks.beforeCompile.tap

#6064 (comment)

@sokra

This comment has been minimized.

Show comment
Hide comment
@sokra

sokra Jan 14, 2018

Member

Hi @johnnyreilly

It looks like you added a custom hook for your plugin. Since the new plugin system adhoc hooks are no longer possible, instead each hook must be registered before being able to call it.

To register your hook add the hook to object.hooks:

const AsyncSeriesHook = require("tapable").AsyncSeriesHook;
compiler.hooks.forkTsCheckerServiceBeforeStart = new AsyncSeriesHook([]);

You can call the hook with:

compiler.hooks.forkTsCheckerServiceBeforeStart.callAsync(function() {...})
// or
promise = compiler.hooks.forkTsCheckerServiceBeforeStart.callPromise()

To make back-compat layer work for your hook:

compiler._pluginCompat.tap("ForkTsCheckerPlugin", options => {
  if(options.name === 'fork-ts-checker-service-before-start')
    options.async = true;
});

Hook is automatically mapped to the camelCased variant. If you don't want this, you can modify the compat layer in the _pluginCompat hook.

Member

sokra commented Jan 14, 2018

Hi @johnnyreilly

It looks like you added a custom hook for your plugin. Since the new plugin system adhoc hooks are no longer possible, instead each hook must be registered before being able to call it.

To register your hook add the hook to object.hooks:

const AsyncSeriesHook = require("tapable").AsyncSeriesHook;
compiler.hooks.forkTsCheckerServiceBeforeStart = new AsyncSeriesHook([]);

You can call the hook with:

compiler.hooks.forkTsCheckerServiceBeforeStart.callAsync(function() {...})
// or
promise = compiler.hooks.forkTsCheckerServiceBeforeStart.callPromise()

To make back-compat layer work for your hook:

compiler._pluginCompat.tap("ForkTsCheckerPlugin", options => {
  if(options.name === 'fork-ts-checker-service-before-start')
    options.async = true;
});

Hook is automatically mapped to the camelCased variant. If you don't want this, you can modify the compat layer in the _pluginCompat hook.

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Jan 14, 2018

Contributor

Thanks @sokra - I'll take a look and report back!

Contributor

johnnyreilly commented Jan 14, 2018

Thanks @sokra - I'll take a look and report back!

@CurtisCBS CurtisCBS referenced this issue Jan 14, 2018

Closed

2018-01-15 #41

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Jan 14, 2018

Contributor

I think that the changes that the html-webpack-plugin is making for webpack 4 probably illustrate what you're advising @sokra. Specificially the changes to index.js and compiler.js:

https://github.com/jantimon/html-webpack-plugin/pull/823/files#diff-168726dbe96b3ce427e7fedce31bb0bc

Might see if I can use that as a basis. Looks like @Graham42 is using AsyncSeriesWaterfallHook where you advise AsyncSeriesHook. I'm guessing that's deliberate.

cc @piotr-oles - relevant for Realytics/fork-ts-checker-webpack-plugin#78

Contributor

johnnyreilly commented Jan 14, 2018

I think that the changes that the html-webpack-plugin is making for webpack 4 probably illustrate what you're advising @sokra. Specificially the changes to index.js and compiler.js:

https://github.com/jantimon/html-webpack-plugin/pull/823/files#diff-168726dbe96b3ce427e7fedce31bb0bc

Might see if I can use that as a basis. Looks like @Graham42 is using AsyncSeriesWaterfallHook where you advise AsyncSeriesHook. I'm guessing that's deliberate.

cc @piotr-oles - relevant for Realytics/fork-ts-checker-webpack-plugin#78

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Jan 14, 2018

Contributor

I've been taking a look at getting ts-loader to support webpack 4. You can see what I've done here:

TypeStrong/ts-loader#710

Here's a Travis build that illustrates issues: https://travis-ci.org/TypeStrong/ts-loader/jobs/328741003

Unfortunately I'm getting a rather cryptic error when I try to run most of the tests:

Unhandled rejection TypeError: _fn0 is not a function
    at AsyncSeriesHook.eval [as callAsync] (eval at create (C:\source\ts-loader\node_modules\tapable\lib\HookCodeFactory.js:24:12), <anonymous>:7:1)
    at AsyncSeriesHook.lazyCompileHook [as _callAsync] (C:\source\ts-loader\node_modules\tapable\lib\Hook.js:35:21)
    at compilation.seal.err (C:\source\ts-loader\node_modules\webpack\lib\Compiler.js:465:30)
    at AsyncSeriesHook.eval [as callAsync] (eval at create (C:\source\ts-loader\node_modules\tapable\lib\HookCodeFactory.js:24:12), <anonymous>:6:1)
    at AsyncSeriesHook.lazyCompileHook [as _callAsync] (C:\source\ts-loader\node_modules\tapable\lib\Hook.js:35:21)
    at hooks.optimizeAssets.callAsync.err (C:\source\ts-loader\node_modules\webpack\lib\Compilation.js:845:35)
    at AsyncSeriesHook.eval [as callAsync] (eval at create (C:\source\ts-loader\node_modules\tapable\lib\HookCodeFactory.js:24:12), <anonymous>:6:1)
    at AsyncSeriesHook.lazyCompileHook [as _callAsync] (C:\source\ts-loader\node_modules\tapable\lib\Hook.js:35:21)
    at hooks.optimizeChunkAssets.callAsync.err (C:\source\ts-loader\node_modules\webpack\lib\Compilation.js:836:32)
    at _err0 (eval at create (C:\source\ts-loader\node_modules\tapable\lib\HookCodeFactory.js:24:12), <anonymous>:11:1)
    at C:\source\ts-loader\node_modules\uglifyjs-webpack-plugin\dist\index.js:257:13
    at step (C:\source\ts-loader\node_modules\uglifyjs-webpack-plugin\dist\uglify\index.js:90:11)
    at C:\source\ts-loader\node_modules\uglifyjs-webpack-plugin\dist\uglify\index.js:113:20
    at tryCatcher (C:\source\ts-loader\node_modules\bluebird\js\release\util.js:16:23)
    at Promise._settlePromiseFromHandler (C:\source\ts-loader\node_modules\bluebird\js\release\promise.js:512:31)
    at Promise._settlePromise (C:\source\ts-loader\node_modules\bluebird\js\release\promise.js:569:18)

My guess is that this is something to do with the changes here:

// loader._compiler.plugin("after-compile", makeAfterCompile(instance, configFilePath));
// loader._compiler.plugin("watch-run", makeWatchRun(instance));
loader._compiler.hooks.afterCompile.tapAsync("ts-loader"), makeAfterCompile(instance, configFilePath);
loader._compiler.hooks.watchRun.tapAsync("ts-loader"), makeWatchRun(instance);

They look fine to me; can anyone offer any thoughts on this? There doesn't seem to be a simple way to diagnose this....

Contributor

johnnyreilly commented Jan 14, 2018

I've been taking a look at getting ts-loader to support webpack 4. You can see what I've done here:

TypeStrong/ts-loader#710

Here's a Travis build that illustrates issues: https://travis-ci.org/TypeStrong/ts-loader/jobs/328741003

Unfortunately I'm getting a rather cryptic error when I try to run most of the tests:

Unhandled rejection TypeError: _fn0 is not a function
    at AsyncSeriesHook.eval [as callAsync] (eval at create (C:\source\ts-loader\node_modules\tapable\lib\HookCodeFactory.js:24:12), <anonymous>:7:1)
    at AsyncSeriesHook.lazyCompileHook [as _callAsync] (C:\source\ts-loader\node_modules\tapable\lib\Hook.js:35:21)
    at compilation.seal.err (C:\source\ts-loader\node_modules\webpack\lib\Compiler.js:465:30)
    at AsyncSeriesHook.eval [as callAsync] (eval at create (C:\source\ts-loader\node_modules\tapable\lib\HookCodeFactory.js:24:12), <anonymous>:6:1)
    at AsyncSeriesHook.lazyCompileHook [as _callAsync] (C:\source\ts-loader\node_modules\tapable\lib\Hook.js:35:21)
    at hooks.optimizeAssets.callAsync.err (C:\source\ts-loader\node_modules\webpack\lib\Compilation.js:845:35)
    at AsyncSeriesHook.eval [as callAsync] (eval at create (C:\source\ts-loader\node_modules\tapable\lib\HookCodeFactory.js:24:12), <anonymous>:6:1)
    at AsyncSeriesHook.lazyCompileHook [as _callAsync] (C:\source\ts-loader\node_modules\tapable\lib\Hook.js:35:21)
    at hooks.optimizeChunkAssets.callAsync.err (C:\source\ts-loader\node_modules\webpack\lib\Compilation.js:836:32)
    at _err0 (eval at create (C:\source\ts-loader\node_modules\tapable\lib\HookCodeFactory.js:24:12), <anonymous>:11:1)
    at C:\source\ts-loader\node_modules\uglifyjs-webpack-plugin\dist\index.js:257:13
    at step (C:\source\ts-loader\node_modules\uglifyjs-webpack-plugin\dist\uglify\index.js:90:11)
    at C:\source\ts-loader\node_modules\uglifyjs-webpack-plugin\dist\uglify\index.js:113:20
    at tryCatcher (C:\source\ts-loader\node_modules\bluebird\js\release\util.js:16:23)
    at Promise._settlePromiseFromHandler (C:\source\ts-loader\node_modules\bluebird\js\release\promise.js:512:31)
    at Promise._settlePromise (C:\source\ts-loader\node_modules\bluebird\js\release\promise.js:569:18)

My guess is that this is something to do with the changes here:

// loader._compiler.plugin("after-compile", makeAfterCompile(instance, configFilePath));
// loader._compiler.plugin("watch-run", makeWatchRun(instance));
loader._compiler.hooks.afterCompile.tapAsync("ts-loader"), makeAfterCompile(instance, configFilePath);
loader._compiler.hooks.watchRun.tapAsync("ts-loader"), makeWatchRun(instance);

They look fine to me; can anyone offer any thoughts on this? There doesn't seem to be a simple way to diagnose this....

@sokra

This comment has been minimized.

Show comment
Hide comment
@sokra

sokra Jan 14, 2018

Member
- loader._compiler.hooks.watchRun.tapAsync("ts-loader"), makeWatchRun(instance);
+ loader._compiler.hooks.watchRun.tapAsync("ts-loader", makeWatchRun(instance));
Member

sokra commented Jan 14, 2018

- loader._compiler.hooks.watchRun.tapAsync("ts-loader"), makeWatchRun(instance);
+ loader._compiler.hooks.watchRun.tapAsync("ts-loader", makeWatchRun(instance));
@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Jan 14, 2018

Contributor

Lemme try

Contributor

johnnyreilly commented Jan 14, 2018

Lemme try

@sokra

This comment has been minimized.

Show comment
Hide comment
@sokra

sokra Jan 14, 2018

Member

for both hooks

Member

sokra commented Jan 14, 2018

for both hooks

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Jan 14, 2018

Contributor

Done - now ts-loader hangs :-(

START:
(node:7792) DeprecationWarning: Tapable.plugin is deprecated. Use new API on `.hooks` instead

and then nothing...

Contributor

johnnyreilly commented Jan 14, 2018

Done - now ts-loader hangs :-(

START:
(node:7792) DeprecationWarning: Tapable.plugin is deprecated. Use new API on `.hooks` instead

and then nothing...

@sokra

This comment has been minimized.

Show comment
Hide comment
@sokra

sokra Jan 14, 2018

Member

I call this Progress...

Member

sokra commented Jan 14, 2018

I call this Progress...

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Jan 14, 2018

Contributor

😄 I've just committed - it's still just hanging though. Should make Travis hang too now 😉

Yup: https://travis-ci.org/TypeStrong/ts-loader/jobs/328747818

Contributor

johnnyreilly commented Jan 14, 2018

😄 I've just committed - it's still just hanging though. Should make Travis hang too now 😉

Yup: https://travis-ci.org/TypeStrong/ts-loader/jobs/328747818

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Jan 14, 2018

Contributor

Yeah I'm wondering if there's something wrong with after-compile. Has anyone else ported to webpack 4 and is using after-compile? I've confirmed that the callback is being called, but after that nothing 😢

  • It's highly possible I'm doing it wrong. Happy to watch and learn!
Contributor

johnnyreilly commented Jan 14, 2018

Yeah I'm wondering if there's something wrong with after-compile. Has anyone else ported to webpack 4 and is using after-compile? I've confirmed that the callback is being called, but after that nothing 😢

  • It's highly possible I'm doing it wrong. Happy to watch and learn!
@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Jan 15, 2018

Contributor

Okay - got some sleep and tried something else. The after-compile hook is running fine as long as I remove the code that reports errors to webpack. These guys: https://github.com/johnnyreilly/ts-loader/blob/88286a185cc5ba1d160aefb22d5f1d3ea5148d8b/src/after-compile.ts#L160

I'll do some more digging but it looks like calling registerWebpackErrors inside after-compile is the problem.

export function registerWebpackErrors(existingErrors: WebpackError[], errorsToPush: WebpackError[]) {
    Array.prototype.splice.apply(existingErrors, (<ErrorOrNumber[]>[0, 0]).concat(errorsToPush));
}

All registerWebpackErrors does is add extra errors to the compilation.errors property. Wow - looking at it, it could really read better!

I did wonder if compilation.errors was one of the Arrays that had become a Set but it seems that's not the case.

Can anyone advise on why adding extra errors to the compilation.errors property in after-compile might be problematic? / how to better to approach this please?

Contributor

johnnyreilly commented Jan 15, 2018

Okay - got some sleep and tried something else. The after-compile hook is running fine as long as I remove the code that reports errors to webpack. These guys: https://github.com/johnnyreilly/ts-loader/blob/88286a185cc5ba1d160aefb22d5f1d3ea5148d8b/src/after-compile.ts#L160

I'll do some more digging but it looks like calling registerWebpackErrors inside after-compile is the problem.

export function registerWebpackErrors(existingErrors: WebpackError[], errorsToPush: WebpackError[]) {
    Array.prototype.splice.apply(existingErrors, (<ErrorOrNumber[]>[0, 0]).concat(errorsToPush));
}

All registerWebpackErrors does is add extra errors to the compilation.errors property. Wow - looking at it, it could really read better!

I did wonder if compilation.errors was one of the Arrays that had become a Set but it seems that's not the case.

Can anyone advise on why adding extra errors to the compilation.errors property in after-compile might be problematic? / how to better to approach this please?

@sokra

This comment has been minimized.

Show comment
Hide comment
@sokra

sokra Jan 15, 2018

Member

compilation.errors is still an Array.

Member

sokra commented Jan 15, 2018

compilation.errors is still an Array.

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Jan 15, 2018

Contributor

Yup - just did the Array.isArray test to be extra sure 😉

Any ideas on how I could on why adding extra errors to the compilation.errors property in after-compile might be problematic? / how to better to approach this please?

Contributor

johnnyreilly commented Jan 15, 2018

Yup - just did the Array.isArray test to be extra sure 😉

Any ideas on how I could on why adding extra errors to the compilation.errors property in after-compile might be problematic? / how to better to approach this please?

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Jan 19, 2018

Contributor

Hey @sokra,

Just wondering if you've any ideas on how I could on why adding extra errors to the compilation.errors property in after-compile might be problematic? Is there an alternate approach for registering errors with webpack that we could use instead?

Much ❤️

Contributor

johnnyreilly commented Jan 19, 2018

Hey @sokra,

Just wondering if you've any ideas on how I could on why adding extra errors to the compilation.errors property in after-compile might be problematic? Is there an alternate approach for registering errors with webpack that we could use instead?

Much ❤️

@sokra

This comment has been minimized.

Show comment
Hide comment
@sokra

sokra Jan 22, 2018

Member

Next alpha version: #6357

Member

sokra commented Jan 22, 2018

Next alpha version: #6357

@webpack webpack locked as resolved and limited conversation to collaborators Jan 22, 2018

@sokra

This comment has been minimized.

Show comment
Hide comment
@sokra

sokra Jan 22, 2018

Member

Thanks for all the feedback

Member

sokra commented Jan 22, 2018

Thanks for all the feedback

@sokra sokra closed this Jan 22, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.