Migrate to webpack 4 and implement production mode #1540

ishitatsuyuki · 2018-03-18T15:06:03Z

HMR doesn't seem possible, as there was no code managing the state transition of the hot update.

Builds are now production by default, --mode development to override.

Signed-off-by: Tatsuyuki Ishi <ishitatsuyuki@gmail.com>

akosyakov · 2018-03-19T07:33:06Z

Could you measure the build performance against the current setup in the dev mode? webpack 4 seems to perform worse than what we have now 12 vs 16s for me. Webpack watching does not seem to work and the documentation should be updated to mention NODE_ENV.

As I said migrating to webpack 4 is a separate issue if you just want to fix #1537 you can open a PR with something like https://github.com/TypeFox/monaco-languageclient/blob/557ef39258b05da48c4e403b39465094c3d3f86a/example/webpack.config.js#L36-L56 for the current setup.

ishitatsuyuki · 2018-03-19T09:19:08Z

Sorry, I can't reproduce the slowdown. The build time on my side shrunk from 15s -> 12s, a 20% improvement.

Watch works on my end. I altered a few transpiled files in the git package and it incrementally rebuilt the bundle in <1s.

Also, some questions:

Someone asked why we should be production by default. Do you have an argument on that?
We currently specify devtool: 'source-map' but webpack's default is eval for dev, none for prod. Would you like to switch to that? Didn't show big speedup on my side though.
Maybe we can add progress option too, but the Theia CLI currently pipes the output so control sequences are not outputted, not overwriting but creating a new line.

akosyakov · 2018-03-19T09:36:29Z

Sorry, I can't reproduce the slowdown. The build time on my side shrunk from 15s -> 12s, a 20% improvement. Watch works on my end. I altered a few transpiled files in the git package and it incrementally rebuilt the bundle in <1s.

Could someone else try and compare? I will try again later, maybe somehow related to my machine.

Someone asked why we should be production by default. Do you have an argument on that?

I wanted to have good defaults for people who deploy apps. Looking at webpack 4 it seems to be as easy as passing --mode production: https://www.valentinog.com/blog/webpack-4-tutorial/#webpack_4_production_and_development_mode. It would be fine with me not have development or production by default then. In examples npm scripts, we need to add --mode development.

We currently specify devtool: 'source-map' but webpack's default is eval for dev, none for prod. Would you like to switch to that? Didn't show big speedup on my side though.

In the dev mode, we should be able to debug the original TS code. In the prod mode do we need source maps at all?

Maybe we can add progress option too, but the Theia CLI currently pipes the output so control sequences are not outputted, not overwriting but creating a new line.

It would be fine with me to fix it. Can we keep it as a separate issue?

marcdumais-work · 2018-03-19T12:58:14Z

Could someone else try and compare?

I can give it a try. Is measuring the time to execute yarn run build in example/browser a valid test for this? Or something else?

ishitatsuyuki · 2018-03-19T12:59:56Z

I can give it a try. Is measuring the time to execute yarn run build in example/browser a valid test for this? Or something else?

That's what I did and should be a good test. Also, please test if the behavior of yarn run watch has changed as well.

marcdumais-work · 2018-03-19T13:37:56Z

I get similar results as @akosyakov :
master: 14s
with this PR: 19s

It seems that with this PR, webpack is quite slow, in watch mode. Let me test a bit more and I'll post an update.

ishitatsuyuki · 2018-03-19T13:38:30Z

Please make sure that you have set NODE_ENV=development.

akosyakov · 2018-03-19T13:40:19Z

ok, i will look into it now, with the watch my bad i rebuilt the wrong package, works nicely!

marcdumais-work · 2018-03-19T14:04:49Z

Thanks for the tip. @ishitatsuyuki . Running export NODE_ENV=development in my shell first this time, made the watch mode appear as fast as usual.

For yarn run build, your PR seems a bit faster than master in my latest run:
master: 14s
with this PR: 12s (I have seen ~14- ish s on some runs)

ishitatsuyuki · 2018-03-19T14:07:52Z

As defaulting to prod has caused some sort of confusion, I'm going to set dev by default on watch, and still make it prod by default for build. This will make the initial npm install slightly longer, but I guess that's acceptable.

akosyakov · 2018-03-19T14:17:57Z

dev-packages/application-manager/src/generator/webpack-generator.ts

@@ -104,7 +105,6 @@ module.exports = {
    },
    devtool: 'source-map',


we should add devtool only if a mode is development

I don't agree. Production doesn't mean errors don't happen, and without sourcemaps it's impossible to report issue with debug info.

Is not it default to webpack 4?

What about cases when users not supposed to see the original code?

I don't think we can satisfy everybody here and would like to go with webpack 4 defaults. If someone wants source maps they can create a new webpack config which merges the default and enables source maps.

Well, webpack's default is eval devtool, which results in not very good debugging experience (you can only see generated code). Though, it's probably good to gate on development in this case.

in the dev mode we know what is the good default

@ishitatsuyuki we discussed it with @svenefftinge, you can leave source maps as is, protection of proprietary extensions is a separate issue

akosyakov · 2018-03-19T14:18:52Z

As defaulting to prod has caused some sort of confusion, I'm going to set dev by default on watch, and still make it prod by default for build. This will make the initial npm install slightly longer, but I guess that's acceptable.

I think we just should add --mode development to build and watch npm scripts in examples package.json.

akosyakov · 2018-03-19T14:22:59Z

dev-packages/cli/README.md

@@ -56,11 +56,15 @@ If you set `next` in your theia config, then Theia will prefer `next` over `late

 To build once:

-    theia build
+    NODE_ENV=development theia build


It should be theia build by default, we could add below that to build in dev mode --mode development should be added.

With webpack 4 we should use --mode instead of NODE_ENV.

akosyakov · 2018-03-19T14:23:47Z

dev-packages/cli/README.md


 In order to rebuild on each change:

-    theia build --watch
+    NODE_ENV=development theia build --watch


the same here --mode development

akosyakov · 2018-03-19T14:24:55Z

examples/electron/package.json

@@ -40,7 +40,7 @@
    "prepare": "yarn run clean && yarn build",
    "clean": "theia clean",
    "build": "theia build",


please add --mode development here too, in Theia repo we are never interested in building production-ready artifacts, the same for the browser example

akosyakov · 2018-03-19T14:26:05Z

dev-packages/application-manager/src/generator/webpack-generator.ts

@@ -45,6 +45,7 @@ module.exports = {
        path: outputPath
    },
    target: '${this.ifBrowser('web', 'electron-renderer')}',
+    mode: development ? 'development' : 'production',


development should be computed based on mode option instead of NODE_ENV and fall back to production if it is not present, similar to webpack 4 defaults

I don't think you can do that.

The issue is that one can send --mode production now and set NODE_ENV to development which installs monaco sources. Although it should not be an issue if we migrate to the latest Monaco.

webpack config is loaded before the arguments get reflected. We may be able to do that if we directly invoke webpack inside JS instead of spawning the CLI.

It should be in process.argv.

kittaakos · 2018-03-19T15:19:17Z

Builds are now production by default, NODE_ENV=development to override.

Perhaps, I should have asked it here instead of the task. Why is the production mode the default? Does that mean if one builds and starts the application from the source the code will be minified and runs in production mode? Why do not we do the other way around?

ishitatsuyuki · 2018-03-19T15:22:57Z

I think that @akosyakov's argument is that the Theia CLI is mostly used by end users. I'm not sure if it's the case.

akosyakov · 2018-03-20T09:09:37Z

dev-packages/application-manager/src/generator/webpack-generator.ts

@@ -22,15 +22,15 @@ export class WebpackGenerator extends AbstractGenerator {
        return this.pck.resolveModulePath(moduleName, path).split(paths.sep).join('/');
    }

-    protected compileWebpackConfig(): string {
+    protected compileWebpackConfig(mode: string | undefined): string {
        return `// @ts-check
 const path = require('path');


The minimal change is to generate the following into the webpack config:

const { mode } = yargs.option('mode', { description: "Mode to use", choices: ["development", "production"], default: "production" });

and pass mode into the configuration.

I don't see why other code should be changed and usage of theia build should stay simple by default.

This is future-proof: we'll no longer be generating config file (and spawning a separate process) after #1552.

We will discuss it later whether we want to implement #1552 and how. For now, it is not necessary.

Signed-off-by: Tatsuyuki Ishi <ishitatsuyuki@gmail.com>

akosyakov · 2018-03-20T10:38:59Z

dev-packages/cli/README.md

@@ -56,11 +56,15 @@ If you set `next` in your theia config, then Theia will prefer `next` over `late

 To build once:

-    theia build
+    theia build --mode development


The main case: the person creates package.json and use theia-cli to build and start an app, not to develop something.

What are you trying to say? How should I modify this?

It should be just theia build.

Then what's the point of having watch documented just below? I have clearly indicated what to use for production, there's no need for additional changes.

ok, i will update docs as the follow up to this PR

akosyakov · 2018-03-20T11:01:51Z

dev-packages/application-manager/src/generator/webpack-generator.ts

+    description: "Mode to use",
+    choices: ["development", "production"],
+    default: "production"
+});


}).argv is missing, otherwise it returns yargs object

Signed-off-by: Tatsuyuki Ishi <ishitatsuyuki@gmail.com>

akosyakov

@ishitatsuyuki thank you!

ishitatsuyuki force-pushed the webpack branch from 26d9f2a to 4a81979 Compare March 18, 2018 15:34

Migrate to webpack 4 and implement production mode

82c66b0

Signed-off-by: Tatsuyuki Ishi <ishitatsuyuki@gmail.com>

ishitatsuyuki force-pushed the webpack branch from 4a81979 to 82c66b0 Compare March 19, 2018 01:34

akosyakov self-requested a review March 19, 2018 07:07

akosyakov reviewed Mar 19, 2018

View reviewed changes

akosyakov mentioned this pull request Mar 19, 2018

Enable development mode by default eclipse-theia/generator-theia-extension#17

Closed

akosyakov reviewed Mar 20, 2018

View reviewed changes

Use development by default for watch and update CLI documentation

5c41459

Signed-off-by: Tatsuyuki Ishi <ishitatsuyuki@gmail.com>

ishitatsuyuki force-pushed the webpack branch 2 times, most recently from db04472 to d182f26 Compare March 20, 2018 09:28

akosyakov reviewed Mar 20, 2018

View reviewed changes

Determine mode from CLI arguments

9cd66ca

Signed-off-by: Tatsuyuki Ishi <ishitatsuyuki@gmail.com>

ishitatsuyuki force-pushed the webpack branch from d182f26 to 9cd66ca Compare March 20, 2018 11:29

akosyakov approved these changes Mar 21, 2018

View reviewed changes

akosyakov merged commit c145b15 into eclipse-theia:master Mar 22, 2018

kittaakos mentioned this pull request Jun 20, 2018

"yarn theia build" works incorrectly for package.json which contains strict defined Theia components version #2137

Closed

JonasHelming mentioned this pull request Feb 16, 2021

Consider to use parceljs instead of webpack #966

Closed

		@@ -104,7 +105,6 @@ module.exports = {
		},
		devtool: 'source-map',

Migrate to webpack 4 and implement production mode #1540

Migrate to webpack 4 and implement production mode #1540

Conversation

ishitatsuyuki commented Mar 18, 2018 • edited

akosyakov commented Mar 19, 2018

ishitatsuyuki commented Mar 19, 2018

akosyakov commented Mar 19, 2018 • edited

marcdumais-work commented Mar 19, 2018

ishitatsuyuki commented Mar 19, 2018

marcdumais-work commented Mar 19, 2018

ishitatsuyuki commented Mar 19, 2018

akosyakov commented Mar 19, 2018

marcdumais-work commented Mar 19, 2018

ishitatsuyuki commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Mar 20, 2018 • edited

Choose a reason for hiding this comment

akosyakov commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kittaakos commented Mar 19, 2018

ishitatsuyuki commented Mar 19, 2018

akosyakov Mar 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Mar 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov left a comment

Choose a reason for hiding this comment

ishitatsuyuki commented Mar 18, 2018 •

edited

akosyakov commented Mar 19, 2018 •

edited

akosyakov Mar 20, 2018 •

edited

akosyakov Mar 20, 2018 •

edited

akosyakov Mar 20, 2018 •

edited