-
Notifications
You must be signed in to change notification settings - Fork 433
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
Conversation
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...
@MarkPieszak does this address lazy loaded routes and AOT with dllplugin? (JavaScriptServices does not). If so, how? |
Hi, great work everybody. Is it safe to upgrade my projects to match latest aspnetcore-angular2-universal. I install it somewhere around February. I really need: Which are known limitations/bugs/problems with latest project code? |
@JohnGalt1717 I have DllPlugin & DllReferencePlugin here above and everything seems to be working ? Works with this repo, which has a lazy-loaded route. |
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! |
@MaklaCof is Lazy Loading working on a fresh pull? My commit for ASP.NET Core 2.0 had a fix for lazy load. |
@JohnGalt1717 did you need to add anything additional to your webpack like before? |
@MarkPieszak should we be creating an issue with running list of features for base repo and add-ons as discussed? |
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!) |
Will this get merged into Master soon? |
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 |
This fixed HMR for me:
|
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 |
This seems to also fix HMR aspnet/JavaScriptServices#1204 |
@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 |
@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. |
I'll know for sure tomorrow on our main large project. If it builds without heap errors we'll be set. |
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) and then add the same line to webpack.client.js my dev build times (HMR) come down from about 35s to 4s! |
@peterdobson that is expected, looking at this inline-source-map is a pretty heavy option |
@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. |
yea we can't help that sadly. i still think we should merge as for smaller projects there is perf gain |
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. |
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' |
Oh definitely, we need all of that. Chunk hashing should be automatically included, that's a must-have for sure! |
Also in the Webpack.config.vendor.js I think it should be this for the serverBundleConfig:
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%. |
(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) |
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",
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. |
@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:
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: AggregateException: One or more errors occurred. (Cannot find module './wwwroot/dist/vendor-manifest.json' I could get over it by manually runing Query : |
@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! 🍾 |
@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! |
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 |
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:
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. |
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 |
It's showing up as Dev all the time, really? I'll take a look. |
@JohnGalt1717 How does your .csproj look like? |
@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. |
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