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

Lazy loading support with Aot #1714

Closed
wants to merge 40 commits into from
Closed

Lazy loading support with Aot #1714

wants to merge 40 commits into from

Conversation

dfaglioni
Copy link

I had to change two major things. First: The SYSTEM_BUILDER_CONFIG to get working the build.bundles.app.ts with bundle (to expose the dependencies), in the previous version the dependencies had a loading error. Two: Included the SystemJS in production, to use on index to load the main app, and exposes for angular to load and compile the lazy module. I removed the app.js from the index.
I created a task to build the lazy bundle, only with theirs dependencies (build.bundles.app.lazy). All folders with + prefix will be considerate lazy modules.
I created two lazy modules, lazy-about and lazy, both has e2e tests to proof the solution.

@markoddi01
Copy link

@dfaglioni I currently have the master branch. With your changes does that mean when i do a serve.prod my lazy load child routes will work? I had to take them out previously because they only worked with npm run start and not npm run serve.prod. So I'm guessing they now work in the bundle? and will this improve the speed of my app?

@dfaglioni
Copy link
Author

That's right, previously they only worked in dev. Now in my branch, I changed to generate a bundle for each lazy module (only with the dependencies that doesn't exists in main bundle). The lazy task considers folders with prefix plus and builds a bundle.
Now you can let only the strictly required to the main app, and the others things goes in lazy modules.
Possibly, you will gain speed on bootstrap, and the size of the main app will not change with the time lapse.

Yet, I have one problem, it have to work with windows!

@markoddi
Copy link

markoddi commented Dec 14, 2016 via email

@dfaglioni dfaglioni changed the title Lazy loading support [WIP] Lazy loading support Dec 14, 2016
@mgechev
Copy link
Owner

mgechev commented Dec 14, 2016

@dfaglioni thanks for the effort! I'd suggest to keep it in a separate branch which we keep in sync with master because a lot of developers will experience problems following the convention that you've introduced.

Also, do you think we can change the prefix from + to something else? It's no longer recommended by the official style guide.

I also did some work regarding bundling for an internal project, I can share my progress here later today.

@mgechev
Copy link
Owner

mgechev commented Dec 14, 2016

Here's my build.lazy-bundles.app.ts task:

import { appendFileSync } from 'fs';
import { join } from 'path';
import * as utils from 'gulp-util';
import * as Builder from 'systemjs-builder';

import Config from '../../config';

const BUNDLER_OPTIONS = {
  format: 'cjs',
  minify: true,
  mangle: true
};

interface Bundle {
  path: string;
  module: string;
}

class BundleNode {
  children: BundleNode[] = [];

  constructor(public path: string) {}

  isParent(node: BundleNode) {
    return node.path.startsWith(this.path);
  }
}

// Prefix tree for proper bundling order
class BundleTree {
  roots: BundleNode[] = [];

  static buildTree(paths: string[]) {
    const tree = new BundleTree();
    paths.forEach((p: string) => {
      if (p === '/') {
        throw new Error('Invalid "/" path');
      }
    });
    paths.sort((a: string, b: string) => a.split('/').length - b.split('/').length)
      .forEach(p => tree.addNode(new BundleNode(p)));
    return tree;
  }

  addNode(node: BundleNode) {
    if (!this.roots.length) {
      this.roots.push(node);
    } else {
      const added = this.roots.some((root: BundleNode) => this.addNodeHelper(node, root));
      if (!added) {
        this.roots.push(node);
      }
    }
  }

  private addNodeHelper(node: BundleNode, context: BundleNode): boolean {
    const added: boolean = context.children.reduce((a: boolean, c: BundleNode) => {
      return a || this.addNodeHelper(node, c);
    }, false);
    if (!added && context.isParent(node)) {
      context.children.push(node);
      return true;
    }
    return added;
  }
}

const normalizeConfig = (bundles: any[]) => {
  bundles = bundles.map((b: any) => b.path);
  return bundles.map((b: string) => {
    if (!b.endsWith('.js')) {
      b += '.module.ngfactory.js';
    }
    return b;
  });
};

const config = JSON.parse(JSON.stringify(Config.SYSTEM_BUILDER_CONFIG));
delete config.paths;
const addExtensions = `
$traceurRuntime = {
  typeof: function (a) {
    return typeof a;
  }
};
System.config(${JSON.stringify(config, null, 2)});
`;

const bundleMain = () => {
  const builder = new Builder(Config.SYSTEM_BUILDER_CONFIG);
  const mainpath = join(Config.TMP_DIR, Config.BOOTSTRAP_FACTORY_PROD_MODULE);
  const outpath = join(Config.JS_DEST, Config.JS_PROD_APP_BUNDLE);
  utils.log('Bundling the bootstrap bundle');
  return builder
    .bundle(mainpath,
      outpath,
      Object.assign({ format: 'umd', sourceMaps: true }, BUNDLER_OPTIONS))
      .then((res: any) => {
        utils.log('The bootstrap bundle is ready!');
        appendFileSync(outpath, `\nSystem.import('${mainpath}.js');${addExtensions}`);
        return res.modules;
      });
};

const bundleModule = (bundle: string, exclude: string[]): Promise<string[]> => {
  utils.log('Bundling module with entry file', bundle);
  let builder = new Builder(Config.SYSTEM_BUILDER_CONFIG);
  let all = join(Config.TMP_DIR, Config.BOOTSTRAP_DIR);
  let bootstrap = join(Config.TMP_DIR, Config.BOOTSTRAP_DIR, bundle);
  const parts = bundle.split('/');
  parts.pop();
  let bootstrapDir = join(Config.TMP_DIR, Config.BOOTSTRAP_DIR, parts.join('/'));
  let expression = `${bootstrap} - (${all}/**/*.js - ${bootstrapDir}/**/*.js)`;
  if (exclude.length) {
    expression += ` - ${exclude.join(' - ')}`;
  }
  return builder
    .buildStatic(
      expression,
      join(Config.JS_DEST, '..', Config.BOOTSTRAP_DIR, bundle),
      Object.assign({}, BUNDLER_OPTIONS, { format: 'umd', sourceMaps: true }))
      .then((res: any) => {
        utils.log('Bundling of', bundle, 'completed!');
        return res;
      });
};

const bundleModules = (roots: BundleNode[], exclude: string[]): Promise<any> => {
  return Promise.all(roots.map((node: BundleNode) =>
    bundleModule(node.path, exclude)
      .then((directExclude: string[]) => {
        return bundleModules(node.children, exclude.concat(directExclude));
      })));
};

/**
 * Executes the build process, bundling the JavaScript files using the SystemJS builder.
 */
export = (done: any) => {
  const config = normalizeConfig(Config.BUNDLES);
  const bundleTree = BundleTree.buildTree(config);
  bundleMain()
    .then((bundled: string[]) => bundleModules(bundleTree.roots, bundled))
    .then(() => {
      let builder = new Builder(Config.SYSTEM_BUILDER_CONFIG);
      return builder
        .buildStatic(join(Config.TMP_DIR, Config.MINI_APP_MODULE),
        join(Config.JS_DEST, Config.MINI_APP_BUNDLE),
                    BUNDLER_OPTIONS);
    })
    .then(() => done())
    .catch((e: any) => done(e));
};

@dfaglioni
Copy link
Author

I can change the prefix for anything.
And about keep in a separate branch, I can create a parameter in seed.config to turn on the lazy and without this, keep the things like before.

@mgechev
Copy link
Owner

mgechev commented Dec 14, 2016

Yes, using a parameter sounds like a better idea. Do you want to provide explicit configuration for the bundles as well? The cons of using prefix is that we introduce another convention, a pro is that we simplify configuration. Still, I think it's a better idea to keep the configuration explicit and use explicit bundle configuration, similar to the example above.

@dfaglioni
Copy link
Author

I created a property on seed config (LAZY_MATCH_EXPRESSION) to use on lazy task for define the folders that are lazy.

@mgechev
Copy link
Owner

mgechev commented Dec 14, 2016

@dfaglioni looks good and I see that the build is passing now. Would you only drop the +s from the directory names?

@markoddi01
Copy link

@dfaglioni Its in node modules. Its not my service.

This is the screen shot of the node module and project config:

screen4

@markoddi01
Copy link

@dfaglioni Seems to be an issue with extracting the node module for metadata which is part of ng2-metadata module.

I've found the file its looking for:

screen5

@dfaglioni
Copy link
Author

@markoddi01 Are you importing like this?
import { MetadataService } from 'ng2-metadata';

@markoddi01
Copy link

@dfaglioni Fixed that by following the bottom part of this link: https://www.npmjs.com/package/ng2-metadata

I now have another issues seems to be with my other module angular2-google-maps:

image

@dfaglioni
Copy link
Author

@markoddi01 Turn off the minify for while

@markoddi01
Copy link

@dfaglioni it now opens the browser and shows console errors. Git bash seems to work fine now that minify was taken out.

The app doesnt show the homepage, just the bootstrap css background blue.

This is the current browser errors:

image

@dfaglioni
Copy link
Author

@markoddi01 Is this returning the index on the get of router_link?

@markoddi01
Copy link

@dfaglioni I'm not sure. Heres some screen shots:

image

image

@dfaglioni
Copy link
Author

@markoddi01 Look at network ...

@markoddi01
Copy link

markoddi01 commented Dec 20, 2016

@dfaglioni network screen:

image

Network Router link response:

image

@dfaglioni
Copy link
Author

@markoddi01 I want to see the response body.

@markoddi01
Copy link

markoddi01 commented Dec 20, 2016

@dfaglioni above in the second screen shot is the response body.

Looks like its returning the index on router link

@dfaglioni
Copy link
Author

@markoddi01
For some reason, the compiler didn't put some dependencies on the final bundle, like router_link.ngfatory. I have to try to simulate here.

@markoddi01
Copy link

@dfaglioni okay, thank you. Let me know if you can.

@markoddi01
Copy link

markoddi01 commented Dec 20, 2016

@dfaglioni I have no idea why it occurs. could it be something to do with the lazy load in the seed file or my project config settings with the 2 node modules I add?

I've tried a few things and still not luck. I will try again tomorrow. Could you recreate my issue for the router link?

@dfaglioni
Copy link
Author

I will try today.

@markoddi
Copy link

@dfaglioni I'm free all evening to try get over this last hurdle.

@markoddi
Copy link

markoddi commented Dec 21, 2016

@dfaglioni any luck? I couldnt get it working. You free today to help?

@dfaglioni
Copy link
Author

@markoddi01 Hey, Sorry, I'm so busy. I'm changing my system to test, but I'm having others types of errors. I'm going to close the PR for while.

@dfaglioni dfaglioni closed this Dec 22, 2016
@markoddi01
Copy link

Hi, @dfaglioni Happy new year. I've not been coding for the last 12 days, in order to allow you to get ahead with the AOT work.

How have you got on with this work?

My branch is still in the above state from when we last spoke. I remember us being very close with the AOT production build.

@amiitn
Copy link

amiitn commented Jan 11, 2017

Hi, @dfaglioni I've tried your solution but still it's not working.
I am getting following error 'TypeError: Cannot read property 'substr' of undefined' when child route is called. Lazy load module bundle is showing in network.

npm run build.prod command is executed successfully without any error. Will you help me in that?

@kobvel
Copy link

kobvel commented Jan 17, 2017

production build does not work in the lazy branch without AOT. Any suggestions about that? We must use AOT if we wanna lazy loading?

@maiphuong
Copy link

Hi @markoddi01 and @@dfaglioni,
I have the same problem and my website need 13-14s to load.It is terrible,Could you have any idea for this one?
Thanks so much

@dfaglioni
Copy link
Author

I suggest to use angular-cli that uses webpack instead systemjs who is the problem of the slow initialize.

@factoidforrest
Copy link

Really sad this never got merged. AOT with lazy loading is the killer feature that is driving everyone to webpack.

SystemJS has some advantages though, like you can build projects completely separately and then load them together and they will share dependencies. That's a big use case for me.

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

Successfully merging this pull request may close these issues.

9 participants