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

WIP: Smarter cdn #1676 #1678

Closed
wants to merge 13 commits into from
Closed

WIP: Smarter cdn #1676 #1678

wants to merge 13 commits into from

Conversation

ralsina
Copy link
Member

@ralsina ralsina commented May 3, 2015

Try to be smarter and automatically use a CDN (or not) without having to do complex code in the templates.

@ralsina
Copy link
Member Author

@ralsina ralsina commented May 3, 2015

Having #1680 would make this possible by making sure bundles are built after copy_assets and before pages

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented May 4, 2015

We could easily avoid dependency injection by using cache/cdnjs and cache/cdncss files that all pages would depend on. The files would be created by the bundles plugin (specified as targets), and dependencies of all page builds. (we could probably pass the lists from bundles to pages without reading the actual files on page generation)

import subprocess

links = {}
data = subprocess.check_output(['links', '-dump', 'https://cdnjs.com/'])
Copy link
Member

@Kwpolska Kwpolska May 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not have to scrape the website. There is a nice .json file we could use with a little parsing (might need an input format change to account for different files and versions): https://github.com/cdnjs/cdnjs/wiki/Extensions%2C-Plugins%2C-Resources#developers

Copy link
Contributor

@da2x da2x May 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be some config with an array of which libraries to include. Hard-coding a few into the default themes is not really flexible.

Copy link
Member Author

@ralsina ralsina May 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Aeyoun I don't understand. The list of available libraries cdnjs supports is quite extensive.

Copy link
Contributor

@da2x da2x May 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ralsina, but there is no setting to tell Nikola what libraries to load. Each library is hardcoded in the theme and not loaded based on a configurable setting.

Copy link
Member Author

@ralsina ralsina May 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Aeyoun I always assumed the theme author chooses what he loads...

@ralsina
Copy link
Member Author

@ralsina ralsina commented May 5, 2015

I'd love @Aeyoun and @Kwpolska 's input on this one :-)

Currently only the bootstrap3 theme is adapted, but you can see how it gets simpler and more robust. Currently the mako template is not jinjify-able tho.

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented May 5, 2015

Currently the mako template is not jinjify-able tho.

This is probably colorbox translations’ fault. I suggest using string formatting, like this (untested):

cdn_url('jquery.colorbox::i18n/jquery.colorbox-{lang}.js',
        use_cdn,
        '/assets/js/colorbox-i18n/jquery.colorbox-{lang}.js',
        lang=colorbox_locales[lang])

(using **kwargs and str.format on the Python side)

@ralsina
Copy link
Member Author

@ralsina ralsina commented Jun 4, 2015

This approach is not convincing to me anymore.

I'll explore alternatives.

Maybe using browserify + requires for "bundling" JS instead of webassets. And https://github.com/jfhbrook/browserify-cdn as the CDN alternative.

@ralsina ralsina closed this Jun 5, 2015
@Kwpolska Kwpolska deleted the smarter-CDN branch May 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants