Skip to content

WIP - Re-sync with JavaScriptServices #376

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

Merged
merged 4 commits into from
Aug 24, 2017
Merged

Conversation

MarkPieszak
Copy link
Member

@MarkPieszak MarkPieszak commented Aug 19, 2017

Main goal is to re-align a bit more with JavaScriptServices to ensure
people coming from there, and wanting to add additional features, have an
easier time syncing with this repo. This also helps ease the build system so that we can improve & make tweaks to both repos simultaneously, by having additional "webpack" differences from here & JSS, done in a separate file.

🍾 🍾 🍾 🍾 🍾

More to come as well...

Will close out:
#307 #318 #296 #294 #271 #267 #230 #161

Partially fix proposed idea in: #286 (project will be split up more-so into PR linkable solutions)

Still in research: #262 (JS heap memory)

cc/
@JohnGalt1717 @isaac2004 @consigliory

Main goal is to re-align a bit more with JavaScriptServices to ensure
people coming from there, wanting to add additional features, have an
easier time syncing with this repo.

Adds back vendor builds,
Adds back fast HMR (but waiting for
aspnet/JavaScriptServices#1204)
AoT faster
Cleans up multiple tsconfigs
etc etc...
@JohnGalt1717
Copy link

@MarkPieszak does this address lazy loaded routes and AOT with dllplugin? (JavaScriptServices does not). If so, how?

@MaklaCof
Copy link

Hi, great work everybody.
I was following all threads for the past few months and I had an impression that Vendor splitting, Tree-shaking, AOT, SSR is impossible due of poor design of Webpack, and bugs in Angular CLI. What's changed.

Is it safe to upgrade my projects to match latest aspnetcore-angular2-universal. I install it somewhere around February.

I really need:
Lazy loading
chunkFilename (webpack configuration to avoid browser caching)
HMR
Production builds
I would like to use:
AOT
TreeShaking
Vendor split
I don't need:
SSR

Which are known limitations/bugs/problems with latest project code?

@MarkPieszak
Copy link
Member Author

@JohnGalt1717 I have DllPlugin & DllReferencePlugin here above and everything seems to be working ? Works with this repo, which has a lazy-loaded route.

@JohnGalt1717
Copy link

Odd. I download the branch, and npm installed and then npm run build:prod and it errored saying I needed "enhanced-resolve".

I added this and magically when it built there were lazy routes with @ngtools which I and pretty much everyone else in the @angular forums have never been able to get working.

I reenabled AOT on my project and added the enhanced-resolve npm library and volia, we have lazy chunks!

Woot!

My example doesn't have the server side rendering. It will be interesting when I port this over to our more complex public site and see if it solves it there as well.

Thanks for all of the hard work!

@isaacrlevin
Copy link
Contributor

@MaklaCof is Lazy Loading working on a fresh pull? My commit for ASP.NET Core 2.0 had a fix for lazy load.

@isaacrlevin
Copy link
Contributor

@JohnGalt1717 did you need to add anything additional to your webpack like before?

@isaacrlevin
Copy link
Contributor

@MarkPieszak should we be creating an issue with running list of features for base repo and add-ons as discussed?

@JohnGalt1717
Copy link

No, completely bone stock other than in the Webpack.config.js (not vendor) I was able to add the PurifyPlugin to it and it didn't crash it, which drastically improves ugilfy's tree shaking ability against Angular etc. (30% smaller chunks!)

@jrmcdona
Copy link
Contributor

Will this get merged into Master soon?

@MarkPieszak
Copy link
Member Author

Yes, probably later toady, just wanted to make a few more improvements on how we build upon the webpack file, anything to make things easier in the future. 👍

HMR is still semi-broken, as something in dotnet core 2.0 changed, so trying to see if I can fix that before merging. If not I'll just merge it anyway, you can at least refresh for now. @jrmcdona

@JohnGalt1717
Copy link

This fixed HMR for me:

    // Before restarting the app, we create a new root element and dispose the old one
    const oldRootElem = document.querySelector('app');
    const newRootElem = document.createElement('app');
    oldRootElem!.parentNode!.insertBefore(newRootElem, oldRootElem);
    oldRootElem.parentNode.removeChild(oldRootElem);
    modulePromise.then(appModule => appModule.destroy());

@jrmcdona
Copy link
Contributor

jrmcdona commented Aug 22, 2017

Hey Guys, what is your best practice/process for merging new changes into our own project branches we have created at work based on this starter project?

I pulled down the project and made some changes to it (i.e. added my own pages, modified some basics) but want to keep syncing with this repo. It is now into our own Git VSO source.

What have you found works best? Cherry picking seems like it would take a long time depending on the change list.

Sorry, if this is a Github newb question.

Thanks

@isaacrlevin
Copy link
Contributor

This seems to also fix HMR aspnet/JavaScriptServices#1204

@isaacrlevin
Copy link
Contributor

@jrmcdona it really depends on how far you strayed from the code. Most of the stuff we are touching are meta, like SSR, HMR, stuff like that. Most of the code that matters is there and the components are just placeholders for whatever you do in your LOB. I think you can probably cherry pick most of it, wouldn't take longer than a few days

@isaacrlevin
Copy link
Contributor

@JohnGalt1717 do you think this PR resolves all the Tree Shaking issues you Jason the past? Would be great to close out all those issues.

@JohnGalt1717
Copy link

I'll know for sure tomorrow on our main large project. If it builds without heap errors we'll be set.

@peterdobson
Copy link
Contributor

peterdobson commented Aug 22, 2017

The aspnet/JavaScriptServices#1204 fixed HMR after porting my project to the latest master too.

I'm not sure if this will have negative knock-on effects, but I noticed with the latest master, in webpack.server.js if I change from:

devtool: 'inline-source-map' (line 11)
to
devtool: 'cheap-eval-source-map'

and then add the same line to webpack.client.js my dev build times (HMR) come down from about 35s to 4s!

@isaacrlevin
Copy link
Contributor

@peterdobson that is expected, looking at this inline-source-map is a pretty heavy option

@JohnGalt1717
Copy link

@isaac2004 AOT and Treeshaking still fails with Java Heap out of memory errors on a large project. So the AOT and Treeshaking issues really aren't fixed. However they're bugs in @ngtools, not this project persay.

@isaacrlevin
Copy link
Contributor

yea we can't help that sadly. i still think we should merge as for smaller projects there is perf gain

@JohnGalt1717
Copy link

JohnGalt1717 commented Aug 22, 2017

I'd suggest a comment on each line that needs to be changed to remove @ngtools and I'd also suggest adding the purify plugin with @angular-devkit/build-optimizer.

And new webpack.optimize.ModuleConcatenationPlugin()

for Webpack 3.

both will shrink stuff significantly. The former by about 20-30% because uglify's tree shaking works better with it, and the later because it eliminates a ton of dupe code. (both vendor and regular for that one, only regular for build-optimizer)

Oh and The CheckerPlugin() should not be called on production while using @ngtools. It's an awesometypescriptloader only function.

@JohnGalt1717
Copy link

JohnGalt1717 commented Aug 22, 2017

ah and in production turn on hashing of the chunks because otherwise you'll get sites that caching will cause errors.

Like this in the client bundle/output: chunkFilename: isDevBuild ? '[id].js' : '[id].[hash].bundle.js'

@MarkPieszak
Copy link
Member Author

Oh definitely, we need all of that.
This was just an initial attempt at trying to maintain the original webpack, but extend it, Ideally all of the additions being in that webpack.additions file.

Chunk hashing should be automatically included, that's a must-have for sure!

@JohnGalt1717
Copy link

Also in the Webpack.config.vendor.js I think it should be this for the serverBundleConfig:

const serverBundleConfig = merge(sharedConfig, {
    devtool: isDevBuild ? 'inline-source-map' : '',
    target: 'node',
    resolve: { mainFields: ['main'] },
    entry: { vendor: isDevBuild ? ['aspnet-prerendering'] : allModules.concat(['aspnet-prerendering']) },
    externals: isDevBuild ? [nodeExternals()] : [],
    output: {
        path: path.join(__dirname, 'ClientApp', 'dist'),
        libraryTarget: 'commonjs2',
    },
    module: {
        rules: [
            { test: /\.css$/, loader: 'ignore-loader' },
            { test: /\.scss$/, loader: 'ignore-loader' }
        ]
    },
    plugins: [
        new webpack.DllPlugin({
            path: path.join(__dirname, 'ClientApp', 'dist', '[name]-manifest.json'),
            name: '[name]_[hash]'
        })
    ],
    node: {
        global: true,
        crypto: true,
        __dirname: true,
        __filename: true,
        process: true,
        Buffer: true
    }
});

The reason for that is that when you do have to build your vendor stuff node will just pull from node_modules in dev so there is no reason to compile a huge vendor.js file for it. This just points the server to the node_modules folder for everything. In production it builds the vendor.js for server side because the node_modules folder doesn't exist.

In testing this cuts full build in dev down by about 80%.

@JohnGalt1717
Copy link

(I'd suggest that JavaScriptServices needs all of these things in there too so I wouldn't separate it out in additions, I'd just do a pull on JavaScriptServices to add it...)

(Not sure I have time right now to do the pull requests and get it done, under the gun but if by this weekend it's still outstanding I'll try and update it)

@JohnGalt1717
Copy link

JohnGalt1717 commented Aug 22, 2017

Actually I got AOT and Tree shaking to complete using these scripts in package.config. We may want to add them and then they'll just work:

"build:dev": "rimraf wwwroot/dist && rimraf ClientApp/dist && node --max_old_space_size=8192 node_modules/webpack/bin/webpack.js --config webpack.config.vendor.js --progress --color && node --max_old_space_size=8192 node_modules/webpack/bin/webpack.js --progress --color",

"build:prod": "rimraf wwwroot/dist && rimraf ClientApp/dist && node --max_old_space_size=8192 node_modules/webpack/bin/webpack.js --config webpack.config.vendor.js --progress --color --env.prod && node --max_old_space_size=8192 node_modules/webpack/bin/webpack.js --config webpack.config.js --progress --color --env.prod"

However when I run this on our major project it once again doesn't create the chunks sadly.

Edit: I got it to work completely! The build commands above fix the memory issue and if you don't get chunks it's because one or more of the modules you have in the nonTreeShakableModules list in the Webpack.config.vendor.js is an angular module. Find it, move it to the treeShakable section and you get chunks emitted. Everything angular MUST be in treeShakable or it blows up with the infamous bug in @ngtools.

@davidsekar
Copy link
Contributor

@MarkPieszak Thanks for this PR. Looks interesting as it aligns up with the default javascriptservices templates.

I tried cloning this branch and run it from Visual studio.

Issue 1:
In NPM restore, based on current package-lock.json I see following issue

  1. npm WARN registry Unexpected warning for https://registry.npmjs.org/: Miscellaneous Warning EINTEGRITY: sha512-ijDLlyQ7s6x1JgCLur53osjm/UXUYD9+0PbYKrBsYisYXzCxN+HC3mYDNy/dWdmf3AwqwU3CXwDCvsNgGK1S0w== integrity checksum failed when using sha512: wanted sha512-ijDLlyQ7s6x1JgCLur53osjm/UXUYD9+0PbYKrBsYisYXzCxN+HC3mYDNy/dWdmf3AwqwU3CXwDCvsNgGK1S0w== but got sha1-Vx4PGwYEY268DfwhsDObvjE0FxA=. (2069 bytes)

  2. Cannot download "https://github.com/sass/node-sass/releases/download/vhttps://registry.npmjs.org/node-sass/-/node-sass-4.5.3.tgz/win32-x64-57_binding.node":
    HTTP error 404 Not Found

After deleting the package-lock.json and doing an NPM install, it installed latest of all packages without any issues.

Is that some thing that you will be looking into before merging this PR ?

Issue 2:
on Visual studio build, the dist folders are not created (webpack dev build is not run) thus it ends up in following error when run using F5

AggregateException: One or more errors occurred. (Cannot find module './wwwroot/dist/vendor-manifest.json'
Error: Cannot find module './wwwroot/dist/vendor-manifest.json'

I could get over it by manually runing npm run build:prod

Query :
tslint is listed as a peer dependency for codelyzer. Can you please include tslint ^5.0.0 as part of packages.json

@MarkPieszak
Copy link
Member Author

@davidsekar Thank you David, I'll definitely add in all of those things, thanks for pointing them out! 💯 💯 I'll make sure to wipe out package lock, and setup the build properly to create the vendor file in VS2017, as well as adding in the HMR fix.

Want to at least merge this once that's done, and then we can make further improvements later! 🍾

@MarkPieszak
Copy link
Member Author

@JohnGalt1717 merging this for now, fixed HMR and VSCode / VS Builds

@davidsekar If you can check master in a few minutes, about to merge this thing. Hopefully the issues you were running into will be fixed now! Thanks again.

Still plenty of improvements I want to make, but this really gets us back inline, and will help dotnet people in general when using either template :)

Next step, HttpClient, and a few other must-have things!

@MarkPieszak MarkPieszak merged commit 5568436 into master Aug 24, 2017
@MarkPieszak MarkPieszak deleted the javascriptservices-reunion branch August 24, 2017 14:15
@Flood
Copy link
Contributor

Flood commented Aug 31, 2017

I don't think the new uglifying steps in webpack.config works. Just tried with the latest and for both:

npm run build:dev and npm run build:prod the file sizes are the same.

main-client.js: 695 kb
vendor.js: 4182 kb

@JohnGalt1717
Copy link

JohnGalt1717 commented Aug 31, 2017

Your vendor.js will shrink only in so much as items aren't in the vendor.js file and in the tree-shakable section of the Webpack.config.js. And you MUST put all Angular components, even third party stuff in the tree shakable section or tree shaking and AOT will not work properly. (and you wont' get any lazy loading chunks built either which will be your first hint)

Also if you add the purify plugin after correcting any issues will help.

At the top of your Webpack.config.js add this:

const PurifyPlugin = require('@angular-devkit/build-optimizer').PurifyPlugin;

Then in your rules do a

            .concat(isDevBuild ? [] : [
                {
                    test: /\.js$/,
                    loaders: '@angular-devkit/build-optimizer/webpack-loader',
                    exclude: /node_modules/,
                    options: {
                        sourceMap: isDevBuild
                    }
                }
            ])

which will add purify to your components (you can add it to node_modules but be very careful because it can use enormous amounts of memory and not get you much more. Angular 4 stuff from google actually has all of the pure tags in their files so there isn't any benefit but 3rd party libraries probably don't.

Then in your client bundle config under plugins where it does the concat for is dev or not in the not right above the uglifyjsplugin line add this:

new PurifyPlugin(),

// And then replace the standard uglify line with this:
  new webpack.optimize.UglifyJsPlugin({
                    beautify: false,
                    comments: false,
                    sourceMap: false,
                    compress: {
                        drop_console: true,
                        dead_code: true,
                        drop_debugger: true,
                        screw_ie8: true,
                        unused: true,
                        warnings: false
                    },
                    mangle: {
                        // Skip mangling these
                        except: ['$super', '$', 'exports', 'require'],
                        screw_ie8: true,
                        keep_fnames: true
                    }
                }),

Make sure you do that in your vendor.js as well.

Finally in your package.config update your build:dev and build:prod commands to look like this:

    "build:dev": "rimraf wwwroot/dist && rimraf ClientApp/dist && node --max_old_space_size=8192 node_modules/webpack/bin/webpack.js --config webpack.config.vendor.js --progress --color && node --max_old_space_size=8192 node_modules/webpack/bin/webpack.js --progress --color",
    "build:prod": "rimraf wwwroot/dist && rimraf ClientApp/dist && node --max_old_space_size=8192 node_modules/webpack/bin/webpack.js --config webpack.config.vendor.js --progress --color --env.prod && node --max_old_space_size=8192 node_modules/webpack/bin/webpack.js --config webpack.config.js --progress --color --env.prod"

And update your .csproj to call npm run build:prod and npm run build:dev instead of having repeated commands inside of the csproj so that if you make any updated params on node etc. they'll be carried over to publish.

You must do the above on any good sized project or node will run out of memory even on a 64 bit processor because it has a 1.7 gb memory limit on 64 bit processors. Our app takes 4.5 gb to compile successfully right now.

When I get time I'll do a pull request and fix all of this stuff up to optimize. My code is 30% of the size of the dev code as a result of this. And with the purify plugin it's about 20% less than without it. Hopefully this points you into the right direction of getting the size under control.

@Flood
Copy link
Contributor

Flood commented Aug 31, 2017

Thank you @JohnGalt1717.

I still believe there is an error on the seed atm. The env parameter is undefined, and therefor isDevBuild is always true.

Tried to make your changes in package.json and that solved the env issue.

Ping @MarkPieszak

@MarkPieszak
Copy link
Member Author

It's showing up as Dev all the time, really? I'll take a look.
There's definitely always more room for improvement in the bundling/minimizing world, ideally it'd be great to have these all as further extensions of the original webpack config!

@Flood
Copy link
Contributor

Flood commented Aug 31, 2017

@JohnGalt1717 How does your .csproj look like?

@JohnGalt1717
Copy link

@Flood Exaclty the same just change the lines where it calls node and Webpack with npm run build:prod and npm run build:dev as the case may be.

@Flood
Copy link
Contributor

Flood commented Sep 1, 2017

image

There you can see that env is undefined.

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

Successfully merging this pull request may close these issues.

8 participants