mgechev / angular-seed

🌱 [Deprecated] Extensible, reliable, modular, PWA ready starter project for Angular (2 and beyond) with statically typed build and AoT compilation
https://mgechev.github.io/angular-seed
MIT License
4.57k stars 1.45k forks source link

Advanced production build #864

Closed mgechev closed 7 years ago

mgechev commented 8 years ago

Currently our production build is quite basic and produces only two bundles:

Which means that we're not taking advantage of:

Incrementally improve the production build by:

Step 1

SamVerschueren commented 8 years ago

Jup sorry, was just a typo here. I have all that and the lazy loading works in development so that doesn't seem to be the problem. The problem is when building the bundles.

Apparently SystemJS has a bundles option in the config. But, by default, angular-seed does not provide a config for production build as it is a static build. So I started adding the config directly in index.html with the bundles property and I get this far that it actually succesfully loads the module and shows this error

Uncaught (in promise): Error: No NgModule metadata found for 'FooModule'.

I might be getting closer to a solution but do I really have to generate that metadata.json file per bundle? Not sure how to do that either so if someone else could point me in the right direction...

RoxKilly commented 8 years ago

@SamVerschueren Oh I see. it's only when you bundle that it stops working. Then you've made it further than I. I gave up on lazy loading (for now) because I couldn't get it to work with my bundling build.

doapp-ryanp commented 8 years ago

Is it possible to gzip all .html,.css,.js as part of build.prod so they can be directly uploaded to static object store (s3+CloudFront)?

We don't support web clients that don't support Accept-Encoding: gzip

I should note that you may be apt to say let CloudFront gzip the assets, however if CF is busy it wont compress.

SamVerschueren commented 8 years ago

@doapp-ryanp Seems like an "easy" extra step that can be written as custom task, no? Seems like a quite specific use case.

doapp-ryanp commented 8 years ago

Yup just didn't know if was already build in (and I was just missing a flag/opt). Seems like a very common usage pattern. I will make a PR at some point to add.

mgechev commented 8 years ago

Hey @doapp-ryanp, I think @SamVerschueren has a point. Lets keep this out of the seed, and maybe write a blog post/wiki article. I recently got rid of 20% of the dependencies and I'm willing to keep this process of shrinking the seed so lets not introduce new dependencies for now.

SamVerschueren commented 8 years ago

@mgechev If you have any docs that I could use in order to make my SystemJS build work with lazy loading, feel free to provide that information. I'm really struggling for hours, if not days, with this problem. Would love to have this in the seed by default.

mgechev commented 8 years ago

@SamVerschueren here's a demo in the official Angular repo. If it still doesn't work after that we can dig deeper into it.

SamVerschueren commented 8 years ago

The thing is that it works while developing (npm start). But the moment I try to create self executing bundles, something fails and I just can't get it to work.

The question is, can we still use builder.buildStatic here or should we use builder.bundle together with a System.import in index.html instead.

mgechev commented 8 years ago

From what I left in the lazy branch I remember that I had to exclude Angular from the production lazy loaded bundles. I think this might have led to some issues in non-recognizing metadata. I've had this issue in the past when I run Angular Universal in Service Workers.

scaryroy commented 8 years ago

@mgechev so are you saying your lazy branch will work if we dont use export *?

mgechev commented 8 years ago

No, the lazy branch doesn't use Google Closure Compiler. The work there is WIP, I'll keep working on it once I come back from connect.js.

mgechev commented 7 years ago

@SamVerschueren here's a working version. Next step is to make it work with AoT.

SamVerschueren commented 7 years ago

@mgechev You are a hero! Thanks for looking into this and finding what caused the issues. I didn't tried it yet but I will when I find some more time.

ruffiem commented 7 years ago

@mgechev I can't make external libraries work in prod on the lazy branch, is it normal (at this stage) ?

mgechev commented 7 years ago

The lazy branch now supports lazy loading with both AoT and JiT compilation. The code there is just proof-of-concept at this point but most likely I'll work on some advanced build in the next a couple of days.

@ruffiem you need to include the library in the closest common parent of the modules using it. It may not be trivial. In my next commits I'll try to introduce a set of conventions aligned with the style guide, for a directory structure which allows easier bundling with systemjs. It should be fun.

mgechev commented 7 years ago

I believe we can close this issue. Improving the production build is an incremental process, dependent on the existing tools. By default SystemJS builder uses rollup anyway, and the lazy loading is something which is already working. In case someone is interested we can discuss it further details in gitter or this issue.

SamVerschueren commented 7 years ago

I thought about creating a new issue for this but it makes more sense to put things here.

I wanted to add treeshaking to my project which started from this seed. So I gave it a go and had something working fairly quickly. BUT, I was importing some CommonJS projects from npm in the following way.

import * as x from 'x';

x();

This is not allowed for Rollup as it will fail with something like Could not call namespace x. So I started investigating this. I got it working by changing that import statement to import x from 'x'; and set the allowSyntheticDefaultImports to true in the tsconfig.json. Production build with AoT and rollup was working perfectly!

But when I ran npm start again, it failed with something like could not find x_1.default as I was now importing the default method. I started to investigate this and came across the inter-format dependencies in the SystemJS docs https://github.com/systemjs/systemjs/blob/master/docs/module-formats.md#inter-format-dependencies. So why wasn't that working for me? I thought SystemJS would hijack the require function and exposes my CJS module as default.

I opened an issue https://github.com/systemjs/systemjs/issues/1502 and got a response. Apparently, for the inter format dependencies to work, you have to transpile your code to system instead of commonjs. But with this came a 3rd issue, module.id does not work anymore as it is CommonJS specific (see issue). I had to change all my module.id calls to __moduleName.

Those steps where required to get Rollup working together with a working dev build.

Quite some changes though, but this got everything working.

@mgechev Yes SystemJS uses rollup, but not sure how good that works. My build with SystemJS was 1.8 MB and when I used rollup directly, I had 900 kB.

mgechev commented 7 years ago

Thanks for the comment! It's great to have this information logged somewhere.

I've considered switching to plain Rollup from SystemJS, however, I'm not sure it's a good idea to move drop SystemJS yet. A lot of people are using plenty of third party CommonJS dependencies and the configuration of rollup can become very complicated.

I've been thinking of having a mixed approach where we support bundling with both SystemJS and rollup. Rollup can be a separate task and we can switch to this build by having something like:

npm run build.prod.exp -- --rollup
npm run build.prod -- --rollup

On the other hand, having tsickle with Google Closure Compiler will produce even smaller bundle and will do everything (bundling, tree-shaking, minification) in a single step...

SamVerschueren commented 7 years ago

Yes switching entirely to Rollup by default wouldn't be a good thing. Especially because this means you would have to use __moduleName instead of module.id and compile to system instead of commonjs. Because all of this, it should be opt-in (if we are going for this).

I added the task below that performs the tree shaking for me in case you're interested.

import * as path from 'path';
import * as gulp from 'gulp';
import * as gulpLoadPlugins from 'gulp-load-plugins';
import * as rollup from 'rollup';
import * as nodeResolve from 'rollup-plugin-node-resolve';
import * as commonjs from 'rollup-plugin-commonjs';
import * as uglify from 'rollup-plugin-uglify';
import * as toPromise from 'stream-to-promise';
import Config from '../../config';
import { templateLocals } from '../../utils';

const plugins = <any>gulpLoadPlugins();

const compileTemplate = () => {
    const stream = gulp.src(path.join(Config.TMP_DIR, '**/*.js'))
        .pipe(plugins.template(templateLocals()))
        .pipe(gulp.dest(Config.TMP_DIR));

    return toPromise(stream);
};

const treeshake = () => {
    return rollup
        .rollup({
            entry: path.join(Config.TMP_DIR, Config.BOOTSTRAP_FACTORY_PROD_MODULE),
            plugins: [
                nodeResolve({
                    jsnext: true,
                    module: true
                }),
                commonjs({
                    include: 'node_modules/**'
                }),
                uglify()
            ]
        })
        .then((bundle: any) => {
            return bundle.write({
                format: 'iife',
                dest: path.join(Config.JS_DEST, Config.JS_PROD_APP_BUNDLE)
            });
        });
};

export = (done: any) => {
    compileTemplate()
        .then(() => treeshake())
        .then(() => done())
        .catch((err: any) => done(err));
};
mgechev commented 7 years ago

Looks good! Let's think how it'll be best to add this as part of the seed and introduce bundling with Rollup in the next a couple of weeks :-).

SamVerschueren commented 7 years ago

Let me know if I can help you out with creating a PR if you know how you want this integrated!

Perezmarc commented 7 years ago

How is the implementation of tree shaking and lazy-loading in production mode going? 😃

ogousa commented 7 years ago

@mgechev I have problems with "lazy" branch (Chrome, Windows 10) while creating production build. First, I have to use

import { HomeComponent } from '../home/home.component'; import { HomeRoutingModule } from '../home/home-routing.module';

instead of

import { HomeComponent } from './home.component'; import { HomeRoutingModule } from './home-routing.module';

The same trick for about module. Otherwise build.lazy-bundles.app.ts cannot find home folder: image

With these changes I can create bundles, but when I copy result to my web server I have this error:

image

Can you please help me? Thanks.

klesky commented 7 years ago

hi am having the same error as @ogousa is having, anyone can shed more light into this?

negberts commented 7 years ago

Same here. Trying to get the Lazy Loading thing working but encountering the same errors (also when using the lazy branch). @mgechev did an update of systemjs or angular maybe caused this?

The error @ogousa got is coming from this line in the bundle.lazy task: appendFileSync(outpath,\nSystem.import('${mainpath}.js');${addExtensions}); mainpath is pointing to tmp dir. why is this included?

negberts commented 7 years ago

@mgechev what settings for MINI_APP_MODULE and MINI_APP_BUNDLE are you using in this gist: https://gist.github.com/mgechev/63d374d51ac846797879a0cdc9c3b97c ?

negberts commented 7 years ago

@ogousa and @klesky did you get any further? Your problem is that in app.js the module is generated as "dist\\tmp\\app\\main.js" but is imported as "dist\tmp\app\main.js". Thats a problem voor SystemJs. After I fixed that I received another error complaining about app.module and when I have a look in chrome to the source of app.module it shows me the content of index.html:

<!DOCTYPE html><html lang="en"><head><base href="/"><meta charset="utf-8"><meta http-equiv="X-UA-Compatible" content="IE=edge"><title>PIM Portal</title><meta name="description" content=""><meta name="viewport" content="width=device-width,initial-scale=1"><link href="https://fonts.googleapis.com/icon?family=Material+Icons" rel="stylesheet"><link rel="stylesheet" href="/css/main.css?1496000579136"></head><body><pim-app>Loading...</pim-app><script>function module(){}</script><script src="/js/shims.js?1496000579131"></script><script src="/js/app.js?1496000579133"></script></body></html>

Really strange...

klesky commented 7 years ago

@negberts i still could not find a solution for this and what do you mean by "module is generated as "dist\tmp\app\main.js" but is imported as "dist\tmp\app\main.js"." ?

negberts commented 7 years ago

"dist\\tmp\\app\\main.js" but is imported as "dist\tmp\app\main.js".

The slashes were removed in my comment. System.js sees \tmp as an escaped t, but the \ should be escaped by \\

darecstowell commented 7 years ago

Having the same issues as @ogousa and @klesky on the lazy branch. That branch could use a bit of love. Are there any plans to update it to Angular 4? My org is creating a fairly large app that benefits more from lazy loading than it does Tree Shaking/Rollup on master.