Skip to content
This repository has been archived by the owner on Mar 26, 2018. It is now read-only.

Fix cdnify #1242

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix cdnify #1242

wants to merge 2 commits into from

Conversation

raphinesse
Copy link

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 raphinesse changed the title [WIP] Fix cdnify Fix cdnify Jan 2, 2016
@wthomsen
Copy link

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
  }
}

@raphinesse
Copy link
Author

@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
Copy link
Author

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

@wthomsen
Copy link

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.

@eddiemonge
Copy link
Member

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

Previously, the script references in index.html that were changed by the cdnify step would later
be overwritten by the usemin task. Now, cdnify runs before useminPrepare and the usemin task is
configured to include references for cdnified scripts right before the actual block replacements.
…rsions

Previously, grunt-google-cdn resolved the angular version requirement of "^1.4.0" to some
1.5 pre-release. Unfortunately there is no release of grunt-google-cdn that includes the relevant
fix. So we now require a specific commit instead of a released version.
@raphinesse
Copy link
Author

raphinesse commented Apr 29, 2016

@eddiemonge Thanks for taking the time.

  • 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.

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

Successfully merging this pull request may close these issues.

cdnify not doing anything.
4 participants