rotundasoftware / cartero

Modular front end development for multi-page web applications.
MIT License
204 stars 25 forks source link

Support for CDN #7

Closed lagoasoft-lucasschmidt closed 11 years ago

lagoasoft-lucasschmidt commented 11 years ago

Hey guys, I`ve been cartero for some time and I am really liking the way code is organized.

I may need to add some conditional CDN, so I think it would be cool if we could add a CDN as a dependency in the bundle.json.

Question is: is it difficult to add? I havent taken a look at the source code yet, I will do it soon, I just wanted to know u guys input regarding this.

dgbeck commented 11 years ago

Definitely sounds like a useful feature and I don't think it would be too difficult to add. There are some intricacies though. Dev vs prod mode is one. You'd just want to use the CDN versions in prod mode correct?

lagoasoft-lucasschmidt commented 11 years ago

Actually my idea is just to use Google, bootstrap CDNs in both modes, thats it, external libraries that I wont touch.

One use case is to use Google Maps APIs in certain pages, but not in others. So, a few functionality in the app would depend on a resource that its in a CDN.

The way the views/javascript is organized, I would need to throw flags to the templates to change which CDN scripts I am using. So it would be nice if we could depend on a external dependency directly into the bundle.json.

I will have to take a look at the source code, since I have some other problems/ideas, I will see if I can take a look this week/weekend. Maybe I can contribute with something.

dgbeck commented 11 years ago

Sounds good!

lagoasoft-lucasschmidt commented 11 years ago

I actually took a look at the code, and I thought it was a pain to understand whats going on. I wasnt able to perform any modifications. Do you guys have any ideas regarding how this can be done?

dgbeck commented 11 years ago

Hi @lucastschmidt. Thank very much for the feedback and the ideas.

Yes, I think this is doable. My thoughts are as follows.

We could probably implement this with single new property in bundle.json, called remoteFiles. The remote files would all need to start with "http://" or "https://"

Then they could be used in the filePriority, devModeOnlyFiles, and prodModeOnlyFiles arrays, just like local files.

We would have to skip preprocessing and concatination of these files.

They would be added to the cartero.json just like normal .js, .css, and .tmpl files, in the appropriate array for their extension.

It would be up to the hook to determine if a file is a local or remote file, and then either resolve the local path appropriately (as it does now), or use the full remote url in the script / link tag.

I'm not sure makes much sense for templates though, as they would need to be fetched by the hook from a remote server and inlined into the page. The hook could conceivably do this, although I'm not sure why anybody would want to go that route, at least not given the current lay of the land.

@lucastschmidt does this sound like it would work for your needs? @go-oleg what do you think? Potential complications?

go-oleg commented 11 years ago

Sounds good to me!

Also, probably a good idea to mark the CDN files as keepSeparate : true while building the Bundle Registry to force them not to be concatenated.

A side thought is that it probably makes sense to have a Bundle be entirely CDN based or entirely not CDN based. You mentioned that you want the CDN files sourced first, which makes sense, but in the grand scheme of things, it might not be possible to guarantee an order specified in filePriority if some of the files are CDN and some aren't and they are interspersed.

lagoasoft-lucasschmidt commented 11 years ago

Well @dgbeck it does sound good.

@go-oleg its hard for me to argue since I do not know the source code as you guys, but my initial idea was to have something completely separated from normal dependencies, since these have a completely different purpose. As you said, its impossible to guarantee order if you order non cdn and cdn files. But thats always going to be a problem when people want to minify/join files, so I dont think its is a problem this software needs to solve.

So, its probably necessary to assume that cdn files come first, or simply give the developers a choice, by being completely separated and them having to include the directives in the templates.

I dont know, in terms of code, if it makes sense to add some sort of hook into existing code to ignore remote files, by detecting using a regex, or to simply add a parallel flow to simply collect them during the normal flow, keeping them in order of dependencies and adding them before all local files.

go-oleg commented 11 years ago

@lucastschmidt ,

I added code for CDN support in the cdn_support branch.

To specify a CDN file in bundle.json, use remoteFiles:

{
    "remoteFiles" : [ "http://code.jquery.com/jquery-1.10.1.min.js" ]
}

example 5 in the examples directory uses CDN for the JQuery bundle.

The cartero-express-hook has been updated to support CDN as well in the cdn_support branch there.

Please let us know if this fits your needs.

-Oleg

lagoasoft-lucasschmidt commented 11 years ago

@go-oleg ,

its looking great. I just did a few tests here, but I found out a problem: If you add a library like google maps, where it doesnt end as .js or .css, the file doesnt get included.

In remoteFiles, we may need to add a different parser of file format. Below its an example:

eg: https://maps.googleapis.com/maps/api/js?key=APIKEY&libraries=geometry,places&sensor=false

dgbeck commented 11 years ago

Hi @lucastschmidt , can you provide a list of all the URLs you want to use as remoteFiles?

lagoasoft-lucasschmidt commented 11 years ago

@dgbeck

{ "remoteFiles": ["https://ajax.googleapis.com/ajax/libs/jquery/1.10.1/jquery.min.js", "https://cdnjs.cloudflare.com/ajax/libs/knockout/2.3.0/knockout-min.js", "https://cdnjs.cloudflare.com/ajax/libs/knockout.mapping/2.3.5/knockout.mapping.js", "https://cdnjs.cloudflare.com/ajax/libs/lodash.js/1.3.1/lodash.min.js", "https://cdnjs.cloudflare.com/ajax/libs/underscore.string/2.3.0/underscore.string.min.js", "https://cdnjs.cloudflare.com/ajax/libs/moment.js/2.1.0/moment.min.js", "https://maps.googleapis.com/maps/api/js?key=APIKEY&libraries=geometry,places&sensor=false", "https://cdnjs.cloudflare.com/ajax/libs/jquery-backstretch/2.0.4/jquery.backstretch.min.js"] }

dgbeck commented 11 years ago

Hi @lucastschmidt,

This looks do-able. We would have to add some funny logic to File::getFileExtension that trims anything to the right of the first "?" and then looks for the last index of "/" or "." to find the extension. A little messy but it would get the job done for this special case.

If you do a pull request we can merge. Also @oleg let us know if we are missing something.

go-oleg commented 11 years ago

Yep, that sounds good for this specific case.

In general though, it seems like there's no way to find out what type of file a CDN URL points to just by looking at the URL.

One approach to get around this could be to separate the remoteFiles into remoteJSFiles and remoteCSSFiles (as @lucastschmidt suggested earlier) or keep as remoteFiles but have each entry contain a url and a type. Basically, it would be up to the user to specify the file type in bundle.json.

Then, while building the Bundle Registry, we could set the type into the File object and in File::getFileType we would check to see if the File already has a type defined, and if so, return that, and otherwise do what we currently do (use the file extension).

lagoasoft-lucasschmidt commented 11 years ago

Hey guys, I think it may be nice for the user to specify what kind of source it is.

I dont think it make sense for us to try to parse an extension based on an URL, since, there may be another use case where the file extension is not even present.

So, I dont know ... separate between remoteJSFiles or remoteFiles with an entry ... it really depends on the way its being implemented the core code, you guys should know better. I dont know which approach cost less, but that should be taken into consideration.

dgbeck commented 11 years ago

Another hybrid option is that we could make remoteFiles an array of strings OR objects w/ type info, and just infer the types of strings how we are doing now. All these options are getting complicated from an API standpoint but the cool thing about that route though is that we can switch to doing it in the future and remain backwards compatible.

For now let's keep inferring the type based on the url. We can trade some internal complexity for a simpler API. If some third party is silly enough to hosts assets on URLs that have no indication of the type then I doubt their assets will be very popular. If that turns out to not be the case we can look at adding object support to remoteFiles.

@lucastschmidt if you want to make a PR along those lines that fits the google map case we can review / merge.

lagoasoft-lucasschmidt commented 11 years ago

Ok @dgbeck, no problem, I will take a look at it later.

lagoasoft-lucasschmidt commented 11 years ago

Hey @dgbeck, https://github.com/rotundasoftware/cartero/pull/12 .

lagoasoft-lucasschmidt commented 11 years ago

Hey @dgbeck, I simplified the implementation. You can take a look at https://github.com/lucastschmidt/cartero/compare/cdn_support

The google-maps case is amazingly edgy, since the path of the url doesnt end with a ".js", but a "folder". I tried to make as generic as possible.

Besides that, I created another branch context_path_support, because I wanted to add support for context-path changes of the app or assets.

dgbeck commented 11 years ago

Hi @lucastschmidt , this looks good! If you open as a PR to cdn_support we can merge this and take it from there. Thank you for the feedback and contribution!

go-oleg commented 11 years ago

CDN support is now available in master (merged in PR #13).