NativeScript / worker-loader

36 stars 20 forks source link

Using global in worker adds unnecessary modules to the worker chunk #5

Closed PeterStaev closed 6 years ago

PeterStaev commented 6 years ago

This is reproducible even with the demo app. If you change the worker to have require("globals") at the top and add global. to onmessage, onerror and postMessage the worker chunk will be 914KB of size, while without those changes the chunk is 6KB of size. This is because the worker chunk does not reuse the vendor chunk and loads in itself all core modules that are needed. This is a really bad design as if you have multiple workers the size will become quite big.

m-abs commented 6 years ago

This is a big problem I would like to see resolved.

Unfortunately the upstream worker-loader also have this issue: https://github.com/webpack-contrib/worker-loader/issues/70

m-abs commented 6 years ago

Workaround, add this to the top of the worker script:

if (global['TNS_WEBPACK']) {
  if (global.android) {
    // without this JavaProxy is missing and we can't import vendor below
    global.require('~/../internal/ts_helpers.js');
  }
  global.require('~/vendor');
} else {
  require('globals');
}

I just tested it with the grayscale worker from here: https://github.com/NativeScript/demo-workers

Works with both webpack bundle and livesync.

PeterStaev commented 6 years ago

Wow, amazing workaround @m-abs ! I'm eager to try it out - I have a production app that i published w/o webpacking just because of this 😄

PeterStaev commented 6 years ago

Well unfortunately @m-abs 's workaround works if you import only the global module. In my case I also have

import * as fs from "file-system";
import * as http from "http";

Having those and using the imported modules results again in an overbloated worker chunk 😞

m-abs commented 6 years ago

I'm thinking another strategy is needed.

What if we add the workers to entry in webpack.config.js and have the worker-loader-plugin generate a file with this content:

require('vendor');
require('worker-name');

This is what GenerateBundleStarterPlugin does for the starter.js.

Unfortunately I don't understand webpack well enough to do it right now.

Adding the worker to the entry generates the "worker-name.js" file as expected, but since vendor isn't loaded first, the script can't be loaded.

PeterStaev commented 6 years ago

Yes! This is how I made it to work in a sample app but I'm trying now to extract the logic in a loader/plugin, as right now it is too manual changes...

PeterStaev commented 6 years ago

Ok, I've made some progress with this: https://github.com/PeterStaev/worker-loader/tree/improved-loader There are couple of steps to configure to make it work: ~1. In your bundle config make sure to register your workers, for example:~

    const viewsContext = require.context("~/", true, /(workers)\/.*\.(js|ts)$/);
    global.registerWebpackModules(viewsContext);

~2. In your webpack.config.js change the name of the common chunk to something else, for example:~

            new webpack.optimize.CommonsChunkPlugin({
                name: ["common"],
            }),

~3. In your webpack.config.js change starter plugin to include the common file as first element:~

            new nsWebpack.GenerateBundleStarterPlugin([
                "./common",
                "./vendor",
                "./bundle",
            ]),

~4. In your webpack.config.js pass the common chunk name to the worker loader plugin:~

new NativeScriptWorkerPlugin({commonChunkName: "common"}),

Then using the loader/plugin from my repo just as you do with the current one will produce two files for every worker: the first one will be the optimized file with all deps loaded from the common chunk and one starter file that requires first the common chunk and then the worker.

Tested this in a production plain {N} app and it works like a charm. ~Haven't tested with an angular app. But because of the split of the vendor chunk into two, snapshots fail to generate 😢 Not sure if there can be some workaround. I've tried producing the worker starter files using the vendor chunk and not splitting into common, but then for some reason the worker does not start. I suspect it is because the vendor has also many UI related modules which fail to load in the worker.~

PeterStaev commented 6 years ago

UPDATE After playing around a bit more I was able to get it to work w/o having to split the common chunk. Also this way makes snapshot work and does not require that much additional changes 🎉 The only required change is that you surround everything in your vendor.ts file in if(!global["__worker"]). So it should look something like:

if (!global["__worker"]) {
    // Snapshot the ~/app.css and the theme
    const application = require("application");
    require("ui/styling/style-scope");
    const appCssContext = require.context("~/", false, /^\.\/app\.(css|scss|less|sass)$/);
    global.registerWebpackModules(appCssContext);
    application.loadAppCss();

    require("./vendor-platform");

    require("bundle-entry-points");
}

The worker loader automatically defines the __worker flag so this change makes UI related things not load in the worker thread. This enables the use of require("./vendor") for the worker.