aurelia / dialog

A dialog plugin for Aurelia.
MIT License
106 stars 115 forks source link

Bug: Lazy loading components causes unnecessary webpack chunks #360

Open pkkummermo opened 5 years ago

pkkummermo commented 5 years ago

I'm submitting a bug report

Please tell us about your environment:

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:

StrahilKazlachev commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Perfect :+1: 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 commented 5 years ago

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

pkkummermo commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

pkkummermo commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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:

jods4 commented 5 years ago

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).