generator-angular icon indicating copy to clipboard operation
generator-angular copied to clipboard

Fix cdnify

Open raphinesse opened this issue 8 years ago • 6 comments

This should resolve #955. Feedback welcome.

At the moment of writing, this PR reduces the vendor.js size of a freshly scaffolded project with default options from 315 kB to 119 kB.

Contrary to the contribution guidelines, there are no tests included with this PR. That is because I could not find any tests regarding (parts of) the generated build process in the current version of generator-angular. So if I were to add tests for this fix I would need directions.

@wthomsen suggested that a fix should also handle cdnified CSS. However, in the current configuration (using google-cdn-data) only JS files are available for inclusion from CDN anyway. From my brief look at the code of google-cdn, I doubt that It actually supports cdnification of CSS files. Consequently this fix only deals with cdnification of JS files.

Since the whole google-cdn ecosystem seems painfully outdated/unmaintained (especially important with the *-cdn-data packages), I thought I might write a replacement using jsdelivr/api, so that the data wouldn't have to be manually synchronized with the respective CDNs. Any thoughts on that would be welcome, too.

raphinesse avatar Dec 30 '15 14:12 raphinesse

It looks like jQuery Mobile and jQuery UI both have css files that can be served via the Google CDN (although I haven't checked the grunt task itself). For completeness, it still seems this pull request should include something to the effect of:

blockReplacements: {
  css: function (block) {
    // repeat JS logic for css
  }
}

wthomsen avatar Jan 13 '16 08:01 wthomsen

@wthomsen Google CDN does also serve CSS files, that is true. However google-cdn-data only contains references to JS files. This is the data source we use right now, and it is unclear if CSS cdnification is supported at all (see PR description). Therefore I would advocate to ignore CSS support for now and add it as a new feature later, if possible.

raphinesse avatar Jan 13 '16 08:01 raphinesse

@eddiemonge I would appreciate some feedback on this PR. Even if it is just "We don't want this.".

raphinesse avatar Apr 26 '16 18:04 raphinesse

This PR should be merged. If not, CDN support should be removed, because it is useless in its current state.

@raphinesse is right about CSS not being picked up by google-cdn-data, so not including it on this PR is fine. It would still be a nice future-proofing step, however.

wthomsen avatar Apr 28 '16 20:04 wthomsen

First, thanks for the work on this. Sorry I've been a bit absentee on this project. Other things have taken priority. I'm trying to get back to it though

  • [ ] Needs a rebase from master
  • [ ] HTML shouldn't be copied from app to dist as it gets processed and moved to tmp
  • [ ] commits should be squashed

eddiemonge avatar Apr 28 '16 21:04 eddiemonge

@eddiemonge Thanks for taking the time.

  • [x] Needs a rebase from master

Done.

  • [ ] HTML shouldn't be copied from app to dist as it gets processed and moved to tmp

I don't quite get this. Would you suggest that files should first be copied from app to tmp, transformed there and then copied to dist? That would only add more code and copying IMHO. What happens now is:

  1. Copy HTML files to dist
  2. Let cdnify work its magic. (It only works in-place AFAIR. Hence the copying.)
  3. Let useminPrepare analyze the cdnified html files.
  4. Business as usual.

So HTML files are just copied to dist earlier on. Could you please elaborate on the problem you see there?

  • [ ] commits should be squashed

I did not squash these two on purpose, since they tackle two different issues that both prevented cdnify to work correctly. Each commit message includes an explanation of what went wrong and how it was fixed. If you still have me rather squash them, I will, of course.

raphinesse avatar Apr 29 '16 09:04 raphinesse