NativeScript / worker-loader

36 stars 20 forks source link

feat: Allow worker to use plugins #39

Closed edusperoni closed 5 years ago

edusperoni commented 5 years ago

This PR allows the worker to use inherited and custom plugins.

Based on worker-plugin.

Fixes #23

edusperoni commented 5 years ago

This PR maintains the same API, but I strongly recommend we switch the rest of the implementation to fall in line with worker-plugin as it also keeps the default new Worker syntax, and makes Typescript integration trivial.

edusperoni commented 5 years ago

~For some reason, my current project (running NS 5.3 and webpack 0.21.2) uses~

~const { AngularCompilerPlugin } = require("@ngtools/webpack");~

~Which made everything work fine. The new format, using ngCompilerPlugin, for some reason throws an error with --bundle without --env.aot:~

ERROR in : TypeError: Cannot read property 'kind' of undefined
        at getDiagnosticHeadMessageForDecoratorResolution

~Enabling aot makes the project build fine.~

~Edit~

~With the demo project, even reverting to 0.21.2 doesn't work. It seems this odd error also happens with angular projects. Maybe it's something in my env. If anyone else could try it, it would be great!~

Edit 2

After some additional testing, it seems to work on a new empty project. So it seems the demo repo is being a little weird. It works fine with aot and throws without. This doesn't happen on any other projects I tested

vcooley commented 5 years ago

I've tried this out and it looks like it works pretty well. However, it seems to be bundling my entire code base into the worker script, rather than only using the code that I import in the worker script itself. Any insight on how to further configure this so that I don't end up double deploying an entire project's worth of code?

As a side note, I've recently needed to implement multithreading in my application and I am astounded that I can't reuse code by default across the Angular and Web Worker sides of my app. This really needs to be addressed to make multithreading viable in my (and I'm guessing many others') use cases.

edusperoni commented 5 years ago

I suggest isolating the shared code (create shared files that are imported in both the worker and app don't have references to other parts of the app). My guess is that tree-shaking in the worker isn't perfect, so you get a lot of stuff broken into small files instead of a big chunk.

Using workers already bundles almost 1MB of mostly unused JS code. Maybe there's a way to spawn the child webpack process without splitting or with a better tree-shaking. Also, try using --env.uglify, which will remove unused code, and see if it improves.

vcooley commented 5 years ago

Awesome, thanks! It turns out I was using a static property on a class that was decorated by and Angular decorator and a few other dependencies, so that's what brought in so many of my files. Removing that cut it in about half. It is still annoying to have so much code duplicated in the form of libraries (I have one major dependency that I need in both threads) but I'm not sure that can be helped, and I haven't tried minifying yet.

vakrilov commented 5 years ago

LGTM! Thanks @edusperoni

About the switching the rest of the implementation, so that we can support the default new Worker syntax - I totally agree it is much better way. Technically it is a breaking change - we just publish v 1.0.0 of this plugin.

I think the best course of action is to merge this PR now and work on new Worker syntax separately (if you are willing to work on an PR - that would be great)

vakrilov commented 5 years ago

Hey @vcooley and @edusperoni I investigated why the generated worker chunks are so big. I tuns out that is because global module requires some polyfills. The one causing most impact is ../ui/dialogs because it has dependencies for many ui modules. I tried to remove the ../ui/dialogs from global and that reduced the size of the worker-chunks from ~2.6mb to ~125KB. On the other hand you don't really need dialogs in workers.

So, I'm thinking that we may introduce a worker-globals package that will still utilize the non-ui polyfills (you might still need timers or xhr in workers). But does not require ui/dialogs. You can then safely use worker-globals in workers and get smaller chunks. How does that seem to you? Do you have any other suggestions?

edusperoni commented 5 years ago

@vakrilov I think it's a great idea! The worker bundle size has bothered me in the past.

Maybe this could be further expanded into breaking each polyfill down to multiple files so we have:

common.ts
xhr.ts
timer.ts
dialogs.ts
...
globals.ts // imports common, xhr, timer, dialogs, etc
worker-globals.ts // imports common xhr and timer

So in the end the developer can just import ".../timers";, if they want to use timers only (or just import the global worker context if no timers are used), further decreasing the worker bundle size. This is probably not a necessity now, but as more polyfills are being added to the core, it might be in the future.