Skip to content
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

Bug: Lazy loading components causes unnecessary webpack chunks #360

Open
pkkummermo opened this issue Aug 28, 2018 · 14 comments
Open

Bug: Lazy loading components causes unnecessary webpack chunks #360

pkkummermo opened this issue Aug 28, 2018 · 14 comments

Comments

@pkkummermo
Copy link

I'm submitting a bug report

  • Library Version:
    2.0.0-rc.x

Please tell us about your environment:

  • Operating System:
    Ubuntu Linux

  • Node Version:
    9.11.1

  • NPM Version:
    6.1.0
  • JSPM OR Webpack AND Version
    webpack 4.17.1
  • Browser:
    all

  • Language:
    TypeScript 3.X

Current behavior:
Upon production build we receive lazyloaded chunks for all dialog components. This is most likely due to the change made in https://github.com/aurelia/dialog/pull/354/files#diff-0ba886733b618be0416988ba431fb6f1L9

Expected/desired behavior:

  • What is the expected behavior?
    The 1.x version automatically bundled within the app.js-chunk due to the loading mechanisms used.

  • What is the motivation / use case for changing the behavior?
    Adding five minimal chunks adds unnecessary overhead and roundtrips to the server and should by default be unlazy.

@StrahilKazlachev
Copy link
Contributor

This is one of the breaking changes.
@jods4 how hard it is to configure webpack not to create separate chunks but bundle them in the one with the dialog core? All those optional resources are now under resources(dist/amd/resources).

@StrahilKazlachev
Copy link
Contributor

The reasoning behind the change is that those resources are not mandatory for the dialog to function.
If you don't need any of them just provide a config callback when setting up the dialog plugin - not calling use .useDefaults nor .useStandardResources.
If you want just some of them call .useResource.
So if you are not using any you should not see any requests for those chunks. I suppose without any additional configs to the build they will still get generated.

@pkkummermo
Copy link
Author

pkkummermo commented Aug 28, 2018

Perfect 👍 I'm only using a subset of the resources anyway (attachFocus mostly).

I tried removing the useDefaults(), and as you suggested it removed the requests for the chunks (even though they are being generated in the dist folder). I don't think there's any overhead just letting them be generated, but it does create some unnecessary files which might clutter a project.

@pkkummermo
Copy link
Author

Btw, what is the correct path to be using to include just some resources, ie attachFocus in my case?

@pkkummermo
Copy link
Author

pkkummermo commented Aug 28, 2018

I found out. What you can't learn by looking at the tests of the project :D (For those whose looking, use the local name of the resource ie useResource("attach-focus") on the config-object for the aurelia-dialog.)

@pkkummermo
Copy link
Author

pkkummermo commented Aug 28, 2018

HOWEVER, this does not bundle it into the production build. My guess is that the bundling magic happens in the PLATFORM.moduleName("<module>")

When I try to add useResource(PLATFORM.moduleName("attach-focus")) it fails (obviously) with Error: Can't resolve 'attach-focus'.
Trying
useResource(PLATFORM.moduleName("aurelia-dialog/resources/attach-focus")) resolves fine, but crashes with main.ts:61 TypeError: resources[name] is not a function. Any tips here?

Solution

Add

.globalResources([PLATFORM.moduleName("aurelia-dialog/resources/attach-focus")])

on your FrameworkConfiguration and it both works locally and in bundled mode.

@StrahilKazlachev
Copy link
Contributor

The sole purpose of all the changes around the resources was to drop the usage of PLATFORM.moduleName, inside the dialog code.
v2 breaking changes are more related to build setup rather than the API.
I can't give example for every build setup, but for aurelia-cli RequireJS/SystemJS projects all needed to include the resources is to add "resources": ["resources/*.js"] to the dialog configuration, in aurelia.json, resulting in:

{
  "name": "aurelia-dialog",
  "path": "../node_modules/aurelia-dialog/dist/amd",
  "main": "aurelia-dialog",
  "resources": ["resources/*.js"]
}

@StrahilKazlachev
Copy link
Contributor

Also the docs have been updated, but are not published to aurelia.io.

@pkkummermo
Copy link
Author

Makes sense to be independent of the PLATFORM-syntax, but as you see it has consequences. Not using the CLI and merely a plain and simple webpack config, this change makes it necessary to do the changes written in my above post to avoid both the default import of the chunks and explicit import of other resources.

@jods4
Copy link

jods4 commented Aug 28, 2018

Some thoughts:

  1. The default webpack logic is that if you dynamically import() it should live in a separate chunk. The reasoning here is that if it's in the same chunk then it's already available in your browser so why not just import it statically?

  2. The answer to this question might be that loading (i.e. running) the module -- even if it's already downloaded and parsed -- could be expensive (not the case here, but anyway) and Webpack gives you an escape hatch with magic comments. Doing import(/* webpackMode: "eager" */ './resources/ux-dialog') will put the module in the same chunk as its parent.

  3. I understand that the goal here was to make those resources optional, so that people who don't use them don't get them.
    I think the best way to do that is to put them in a different file that users import during their configuration (then it's part of their build), or not (then it's not).
    Something like (user code):

import resources from "aurelia-dialog/default-resources";

dialogConfig.useResources(resources);

@StrahilKazlachev
Copy link
Contributor

StrahilKazlachev commented Aug 28, 2018

For v2 I was looking for some kind of middle ground. My first approach was 3. But from project/build setup that turned complicated, personal opinion. First the resources got dropped from being reexported, so now you have to get them like import {UxDialog} from 'aurelia-dialog/resources/ux-dialog';. Second, without any additional configuration/mapping that would resolve to something like node_modules/aurelia-dialog/resources/ux-dialog.js. And the actual location would be aurelia-dialog/dist/${target_build}/resources/ux-dialog. So in that case we would drop the mapping of aurelia-dialog => node_modules/aurelia-dialog/dist/${target_build} to the consumer - decided against for v2.
My goal forv2 is to keep the v1 API but also make any necessary changes to integrate the usage of class ref API, dropping the usage of PLATFORM.moduleName.

@jods4 with that said do you think we can go with /* webpackMode: "eager" */ and still be able to exclude those resources from the build if they are not used, without resorting to any black magic?
btw, thanks for the feedback!

@jods4
Copy link

jods4 commented Aug 28, 2018

If you go with webpackMode: "eager" they will be included in your bundle, in the same chunk as the parent module.

Then users who want to drop them will have to resort to some Webpack plugin (does that count as black magic?)

Note that you can avoid mappings if you publish those modules at the root of your npm module instead of dist.

@StrahilKazlachev
Copy link
Contributor

StrahilKazlachev commented Aug 28, 2018

@jods4 I don't see how they can be moved to the root, we do have different builds amd/native-modules/...
If we don't go for webpackMode: "eager" would users who want to include the resources, in the same chunk as the parent module, also need a plugin?

What I want to understand is which case would require less work/be more simple:

  • not using webpackMode: "eager" and configuring the build to include the resources in the same chunk when used
  • using webpackMode: "eager" and configuring the build to exclude the resources when not used

@jods4
Copy link

jods4 commented Aug 28, 2018

Right... silly me, I was in a context where I just deliver standard ES to everyone. Having mutliple distributions make things more complicated of course.

I guess merging chunks would be a plugin, too.
There are a few optimize options that can aggressively merge chunks (e.g. based on minimum size) but they never merge something into the entry chunk (to reduce startup time).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants