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

Bundle and Minify output #88

Closed
NiXXeD opened this issue Jan 18, 2016 · 12 comments
Closed

Bundle and Minify output #88

NiXXeD opened this issue Jan 18, 2016 · 12 comments

Comments

@NiXXeD
Copy link
Member

NiXXeD commented Jan 18, 2016

  • Bundle
  • Minify
  • Template cache
@smathson
Copy link
Contributor

Hey man, looking for an excuse to play around with ng2/TS and remembered you were working on this. Thought I might try to help out a bit if you are down for PRs.

Did you have any thoughts on a direction for this? I've done just straight concat/minify with gulp in the past, but Webpack seems like a much better approach these days if you're down for that.

@NiXXeD
Copy link
Member Author

NiXXeD commented Jan 23, 2016

I think the main goal is that it would bundle, minify (only for prod), and package templates as well. Then we could get rid of systemjs that I'm using currently.

I would lean towards webpack. I spent a little time playing with Browserify with tsify, but didn't get it to a working point.

@smathson
Copy link
Contributor

Cool. Have an opinion about whether to continue to CDN external libs? Don't know if that was a technical or financial choice.

@NiXXeD
Copy link
Member Author

NiXXeD commented Jan 23, 2016

Currently, it's more of a technical than financial choice. The angular2 bundles I have locally don't seem to work properly in all scenarios. If I choose the angular2.min.js, I actually get errors loading it in the browser, and it's 500kb. The CDN minified one is like 111kb, and no errors. Not sure what's going on there, but probably a beta thing.

Ideally we would have the option to switch back to including them in the bundle later. Though I'm not really opposed to just leaving it CDN.

@smathson
Copy link
Contributor

Okay, I'll play around with it both ways and see what's up. I know one of the benefits of bundling them from local deps is being able to depend on only the submodules that you use instead of the entire lib. Like with lodash being able to do...

import {
    map,
    find
} from 'lodash';

...and have webpack only include the parts of the lib actually used.

@NiXXeD
Copy link
Member Author

NiXXeD commented Jan 23, 2016

Yea, that's another issue I have on here that I wanted to solve at one point. If you can tackle that all, go for it.

@smathson
Copy link
Contributor

Hey man, made some progress on this over the weekend and interested in getting your feedback before I keep going: https://github.com/smathson/chatty/commits/bundle-with-webpack

TLDR: Should be able to pull that branch, npm prune; npm install and run npm run start to kick off the webpack development server with livereload and sourcemaps for TS/SCSS. Everything seems to be running properly, but I'm obviously not as familiar with the codebase as you are so might have missed something.

Done so far:

  • Set up webpack and webpack-dev-server for development bundling
  • Split package.json dependencies into dependencies and devDependencies
  • Import/require templates and styles so that webpack pulls them in and bundles them properly
  • Font Awesome and the Intl polyfill are still being CDNed, everything else is being imported from node_modules
  • SourceMaps set up for TS and SCSS files, currently inline instead of generating individual .map files

Still to do:

  • Add back in linting for SASS
  • Nuke unnecessary gulp tasks (should really only need gulp-awspublish after this)
  • Add production build configuration with minification, gzip, cache busting and no sourcemaps
  • Investigate vendor bundle size. In development vendor.bundle.js is 14MB(!) with inline sourcemaps. I know over half of that is due to sourcemaps, but hoping the rest is just lack of minification. If not, I'll need to do some research around whether dependencies are being duplicated somehow during bundling. Could also be related to NG2 dependencies not being optimized yet (which I know they are working on now). If things don't end up a reasonable size for production, we can always fall back to CDNing some libs for now.

Keep in mind that I tend to commit early and often with wip commits while developing and frequently rebase to squash, so don't trust the history on that branch or attempt to do any lasting work off of it. =]

Let me know if you have any questions/comments/concerns.

@NiXXeD
Copy link
Member Author

NiXXeD commented Jan 25, 2016

Did a cursory code review and it looks good.

A few thoughts

  • npm scripts should have clean, build, publish still after you're finished, not sure if this is what you meant by production configuration. I'll clarify either way just to be sure.
    • clean should wipe out any build dirs
    • build would create the output directory (which could be served by any local web server)
    • publish for now would do clean, build, and (for now) gulp publish. I'll probably drop gulp out though since it's not needed after webpack. I use this script a lot locally when pushing up to S3.
  • I noticed you are using plain node-sass now, any reason for the change? I had decent luck with PostCSS before, and we had moved away from plain sass as PostCSS gave us a lot of options.
  • I notice you're using require for pulling the templates into the components. Can't this be done using ES6 imports? Ex:
import * as template from './thread.html'

@Component({
    selector: 'thread',
    template,
    directives: [Comments, Post, ReplyBox]
})

Good start!

@smathson
Copy link
Contributor

  • Yep, all those scripts will make a comeback with their webpack equivalents. I'll leave the existing gulp publish task in place and let you replace it with whatever makes the most sense for you and then you can nuke gulp.
  • Nope, no specific reason just wasn't as familiar with PostCSS and thought it was SASS we were pulling in. Should be simple to replace, I'll do that along with adding style linting back in.
  • Good catch, I'll bet you're right about the templates. Will try that.

@NiXXeD
Copy link
Member Author

NiXXeD commented Jan 27, 2016

I've removed the gulp-publish task and associated dependency. You should be able to fully remove gulp now.

@NiXXeD
Copy link
Member Author

NiXXeD commented Feb 15, 2016

Any update on this? Pretty excited to get this bundled as it will reduce my s3 usage quite a bit. Also been avoiding working on anything big so you don't get merge conflicts. Anything I can help with?

@smathson
Copy link
Contributor

Yeah sorry man, right in the middle of a job transition at the moment and haven't had enough free time lately to finish this. Got the styles switched back over to PostCSS and added style linting back in, so just need to do the production configuration now. Got some free time today to work on this but don't want to hold you up on anything so if I don't make enough progress I'll at least try to get things into a good state to hand off for you to finish.

Expect a PR later today!

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

No branches or pull requests

2 participants