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

Better BootstrapCDN output #56

Closed
jimaek opened this issue Nov 21, 2014 · 29 comments
Closed

Better BootstrapCDN output #56

jimaek opened this issue Nov 21, 2014 · 29 comments
Assignees

Comments

@jimaek
Copy link
Member

jimaek commented Nov 21, 2014

More info jsdelivr/bootstrapcdn#460

@markhalliwell
Copy link

If there were an easier way to extract the necessary information for the variation of bootstrap & "themes" (from Bootswatch) it would help reduce/eliminate the following code:

http://cgit.drupalcode.org/bootstrap/tree/includes/cdn.inc#n121

Especially the need for this:
http://cgit.drupalcode.org/bootstrap/tree/includes/cdn.inc#n224

@markhalliwell
Copy link

#15 would semi-help with this I believe, but this issue goes a little further in "filling out" the missing/required assets... which may, incidentally, be more of a BootstrapCDN issue?

@UnbounDev
Copy link
Contributor

@jimaek we can do this but we'd essentially have to replicate @MarkCarver's parsing logic. This would be done in api-sync and would result in an enhanced bootstrap/bootswatch assets array, ala

{
        "version": "2.1.0",
        "themes": [
          "amelia",
          "cerulean",
          "cyborg",
          "journal",....
        ],
        "files": [
          "amelia/bootstrap.min.css",
          "amelia/bootstrap.no-icons.min.css",
          "cerulean/bootstrap.min.css",
          "cerulean/bootstrap.no-icons.min.css",
          "cyborg/bootstrap.min.css",
          "cyborg/bootstrap.no-icons.min.css",
          "img/glyphicons-halflings-white.png",
          "img/glyphicons-halflings.png",
          "journal/bootstrap.min.css",
          "journal/bootstrap.no-icons.min.css",
          "readable/bootstrap.min.css",
          "readable/bootstrap.no-icons.min.css",
          "simplex/bootstrap.min.css",
          "simplex/bootstrap.no-icons.min.css",
          "slate/bootstrap.min.css",
          "slate/bootstrap.no-icons.min.css",
          "spacelab/bootstrap.min.css",
          "spacelab/bootstrap.no-icons.min.css",
          "spruce/bootstrap.min.css",
          "spruce/bootstrap.no-icons.min.css",
          "superhero/bootstrap.min.css",
          "superhero/bootstrap.no-icons.min.css",
          "united/bootstrap.min.css",
          "united/bootstrap.no-icons.min.css"
        ]
      }

Unless I'm misunderstanding the issue, this would be a singular case that does not apply generically to any other library w/i the bootstrap cdn.

@jimaek
Copy link
Member Author

jimaek commented Jun 3, 2015

@UnbounDev How would the response format differ from our normal one in v2?

@UnbounDev
Copy link
Contributor

The structure of assets would differ; ala ~

  • Normal v2 assets
{
  mainfile: "",
  files: []
}
  • Bootswatch library v2 assets
{
  files:[],
  themes:[
    <themeName_1>,
    <themeName_2>,
    ...
  ]
}

@MarkCarver is that the format you're looking for?

@markhalliwell
Copy link

Sorry, I've been a little busy lately. I'll try to word this a little differently and give some examples.

I don't think these need a dedicated themes category per se. They can, in theory, still remain in assets (like #15 suggests).

I don't think it really has much to do with how jsDelivr actually provides them via the API exactly, but I could be wrong. I still think the issue really lies in how BootstrapCDN provides its assets to begin with. That is why I had originally created jsdelivr/bootstrapcdn#460 instead of one in jsDelivr's issue queue.

The biggest pain-point is that, while yes there are "themes" provided, they really lack any sort of context in which dependents are necessary to get said "theme" actually working.

The problem is further compounded by the fact that "themes" are also different per version, i.e.
the "paper" theme was introduced in versions 3.2.0 and higher, while the "amelia" theme was actually removed starting with versions 3.3.1 and higher.

For instance, for one to actually get the "paper" theme working (properly), they have to load: bootstrap.js in addition to the paper.css (which have different base paths too btw).

{
  "3.3.4": {
    "paper": {
      "css": ["//cdn.jsdelivr.net/bootswatch/3.3.4/paper/bootstrap.css"],
      "js": ["//cdn.jsdelivr.net/bootstrap/3.3.4/js/bootstrap.js"],
      "min": {
        "css": ["//cdn.jsdelivr.net/bootswatch/3.3.4/paper/bootstrap.min.css"],
        "js": ["//cdn.jsdelivr.net/bootstrap/3.3.4/js/bootstrap.min.js"]
      },
      "title": "Paper"
    }
  }
}

It's also even a little weirder when someone chooses the bootstrap_theme (the one provided by Bootstrap itself, not Bootswatch). The main bootstrap.css file must be loaded first too:

{
  "3.3.4": {
    "bootstrap_theme": {
      "css": [
        "//cdn.jsdelivr.net/bootstrap/3.3.4/css/bootstrap.css",
        "//cdn.jsdelivr.net/bootstrap/3.3.4/css/bootstrap-theme.css"
      ],
      "js": ["//cdn.jsdelivr.net/bootstrap/3.3.4/js/bootstrap.js"],
      "min": {
        "css": [
          "//cdn.jsdelivr.net/bootstrap/3.3.4/css/bootstrap.min.css",
          "//cdn.jsdelivr.net/bootstrap/3.3.4/css/bootstrap-theme.min.css"
        ],
        "js": ["//cdn.jsdelivr.net/bootstrap/3.3.4/js/bootstrap.min.js"]
      },
      "title": "Bootstrap Theme"
    }
  }
}

These JSON examples are just examples from the generated output of the links mentioned above.

Here is a the full list: http://pastebin.com/TqeLDgJc

Ignore the fact that the file is nested under "jsdelivr -> themes", that is just how I have currently configured the Drupal base-theme, "themes" could easily just be "assets".

IMO, what really needs to happen is that BootstrapCDN just needs to split out it's different "themes" into a proper structure based on:

  1. Versions
  2. Dependent assets
  3. Min/non-min support

@markhalliwell
Copy link

Oh, I guess I should also mention that I'm not entirely expecting all those variations met per se, more so that they are, at the very least, nested under the version and each respective "theme" name (as well as including the missing assets too).

I am certainly capable/willing to parse out the additional variations of css/js/min based on matching the endings of the files (to get my desired structure):

{
  "assets": {
    "3.3.4": {
      "bootstrap_theme": [
        "//cdn.jsdelivr.net/bootstrap/3.3.4/css/bootstrap.css",
        "//cdn.jsdelivr.net/bootstrap/3.3.4/css/bootstrap-theme.css",
        "//cdn.jsdelivr.net/bootstrap/3.3.4/js/bootstrap.js",
        "//cdn.jsdelivr.net/bootstrap/3.3.4/css/bootstrap.min.css",
        "//cdn.jsdelivr.net/bootstrap/3.3.4/css/bootstrap-theme.min.css",
        "//cdn.jsdelivr.net/bootstrap/3.3.4/js/bootstrap.min.js"
      ],
      "paper": [
        "//cdn.jsdelivr.net/bootswatch/3.3.4/paper/bootstrap.css",
        "//cdn.jsdelivr.net/bootstrap/3.3.4/js/bootstrap.js",
        "//cdn.jsdelivr.net/bootswatch/3.3.4/paper/bootstrap.min.css",
        "//cdn.jsdelivr.net/bootstrap/3.3.4/js/bootstrap.min.js"
      ]
    }
  }
}

@markhalliwell
Copy link

Also, I realize that my examples have the following base paths in them:
//cdn.jsdelivr.net/bootstrap/3.3.4/
//cdn.jsdelivr.net/bootswatch/3.3.4/

Currently these aren't included, but maybe they should be? I'm not entirely sure, especially given that these are technically two different repositories/libraries? I'm not sure how that actually works, because I thought it was provided by a single provider: BootstrapCDN.

@UnbounDev
Copy link
Contributor

Thanks for the details!

The JsDelivr data is not taken from the bootstrap api, rather, it is taken by scraping the relative github repo (an example being this folder relating to the bootswatch 3.3.4 version). This scrape is currently generic across all CDN's served by the API, as is the format of the associative assets field, where the goal is to provide component-style information on the desired projects and the files therein. While this structure works well for most libraries it fails a bit in the case of bootswatch b/c each bootswatch version is actually a collection of theme libraries.

I do not see malforming the assets field as an appropriate sol'n to the issue (as this is really pretty specific to the bootswatch library), I do see providing additional fields to compensate for the structures limitations in this case; e.g.

{
  "assets": {
    "3.3.4": {
      "files": [<files as usual>],
      "bootstrap_theme": [
        "css/bootstrap.css",
        "bootstrap-theme.css",
        "js/bootstrap.js",
        "css/bootstrap.min.css",
        "css/bootstrap-theme.min.css",
        "js/bootstrap.min.js"
      ],
      "paper": [
        "bootstrap.css",
        "js/bootstrap.js",
        "/bootstrap.min.css",
        "js/bootstrap.min.js"
      ]
    }
  }
}

Where each additional theme field maintains a list of files w/i that theme library. I don't see building the full url as advantageous given that the API's intent is to provide suitably formatted components not full paths to a given CDN.

@jimaek thoughts?

@markhalliwell
Copy link

Yes, I thought that's what jsDelivr did (scrapping), but it is still just scrapping the single BootstrapCDN's repo (just in the bootswatch sub-folder).

i.e.:
https://github.com/MaxCDN/bootstrap-cdn/tree/develop/public/bootswatch
vs.
https://github.com/thomaspark/bootswatch

That's why I was thinking this is still somewhat of a BootstrapCDN issue (to provide the proper data structure to begin with)?

Yes, I agree about the paths (I hadn't considered different CDNs), but http://api.jsdelivr.com/v1/bootstrap/libraries doesn't include a base (or default) CDN base path at all.

Which is something that still confuses me is the disconnect between the api:
http://api.jsdelivr.com/v1/bootstrap/libraries (which includes all the sub-directories from BootstrapCDN)

and the actual path to said resource:
https://cdn.jsdelivr.net/bootstrap vs. https://cdn.jsdelivr.net/bootswatch

Why are these different? Am I using the wrong base path/URL? Isn't this kind of vital information to an API, even as an abstract top level property?

@jimaek
Copy link
Member Author

jimaek commented Jun 4, 2015

Cant we just consider each theme as a separate project and keep the exact same structure and format for all CDNs?

@UnbounDev
Copy link
Contributor

It's a good idea but we'd be losing the ability to see that those libraries actually belong to bootswatch, they'd have to be prefixed w/ something (i.e. bootswatch.<themeName>) and it would mean parsing all bootswatch theme library versions into their own library bucket on the back end (in api-sync). All of that is making this feel more like a hack.

@jimaek
Copy link
Member Author

jimaek commented Jun 4, 2015

I guess its bootswatch.<themeName> vs special structure just for Bootstrap.
I personally vote for bootswatch.<themeName> what everyone else thinks?

@UnbounDev
Copy link
Contributor

@MarkCarver I'll defer judgement to you as you have the most insight into this use case; would you find a special structure just for the bootswatch library or the bootswatch.<themeName> approach more suitable?

@MartinKolarik
Copy link
Member

@jimaek so we are going with each theme as a project named bootswatch.<themeName>, right?

@jimaek
Copy link
Member Author

jimaek commented Jan 26, 2016

yeah, lets do that

jimaek added a commit to jsdelivr/api-sync that referenced this issue Jan 26, 2016
@markhalliwell
Copy link

@jimaek I'm pretty sure v1 just got borked (again): http://api.jsdelivr.com/v1/jsdelivr/libraries/bootstrap

X-ref: https://www.drupal.org/node/2657138

It no longer has the bootswatch stuff from BootstrapCDN

Shouldn't any changes made have only been applied to v2 stuff? I'm a little confused with how y'all are defining v1 and v2 stuff TBH. This isn't the first time that stuff like this has happened.

@markhalliwell
Copy link

Sorry, wrong URL above:

http://api.jsdelivr.com/v1/bootstrap/libraries has the new bootswatch.<theme> entries, but it's v1! These changes should ONLY be showing up in v2... whatever that URL is...

Also, sorry for the late reply, I did not see #56 (comment) till just now.

bootswatch.<themeName> vs special structure just for Bootstrap

bootswatch.<themeName> is fine by me (I just saw the new structure). I can parse the themes individually easily enough by matching bootswatch. from the key.

@MartinKolarik
Copy link
Member

@MarkCarver you are right. All versions use the same datastore and it isn't really possible to return a different list of projects for each API version (ok, it certainly is possible, but not without extremely ugly hacks), so this affected v1 as well.

I recognize that this is somewhat a breaking change, but I also don't think returning a different set of projects depending on whether you use v1 or v2 is a good idea. I think we could add all those theme files back to bootswatch, while also keeping them as individual projects. What do you think?

@markhalliwell
Copy link

so this affected v1 as well

Um... no! The whole reason APIs have "versions" is to assist with not breaking other software that consumes it (which is also versioned and deployed itself).

https://www.drupal.org/project/bootstrap has >94,000 installs and y'all just broke every single one of those install's UIs (which relies on this API to provide versions and themes in a select dropdown https://www.drupal.org/node/2657138).

I think we could add all those theme files back to bootswatch, while also keeping them as individual projects.

That would certainly (temporarily) "fix" the problem, but you're kind of also missing the point of what API "versions" are. You shouldn't be fiddling around with existing (stable) APIs. That's just ludicrous.


I'll be honest, I'm a little miffed by such a cavalier response to this.

@markhalliwell
Copy link

it isn't really possible to return a different list of projects for each API version (ok, it certainly is possible, but not without extremely ugly hacks)

What is the point of the v1 in the URL then??? I know it may not be "easy" on the backend, but a simple change in the URL to v2 and bam, different results.

@MartinKolarik
Copy link
Member

but you're kind of also missing the point of what API "versions" are.

The API version gives you a guarantee of where you can get the data (i.e., endpoints), and what shape the data are (i.e., response model). The data itself are (almost) always dynamic. If you were using GitHub API to get a list of files in a repository and the repository got deleted, you would end up in the same situation.

The only difference here is that with CDNs, once the files get published, we expect them to be there indefinitely. That's why I said removing them can be seen as a breaking change in this case, but new projects can be (and are being) added all the time...

@markhalliwell
Copy link

what shape the data are (i.e., response model)

Exactly........ which is what changed.........

What I'm talking about has absolutely nothing to do with the "projects" or "files" themselves. The underlying BootstrapCDN/files really hasn't changed all that much, for a long time actually.

It has everything to do with how this "API" parses and reconstructs BootstrapCDN's assets into a new data modal. This process is what constitutes jsDelivr as "authoritative" since YOU determine where the parsed and reconstructed files "show up" in this compiled data model.

This is why existing code broke when you suddenly changed what it expected from "v1".

This has happened before (#94). This kind of behavior is not acceptable from an "API".

If you can't clear up what "v1" and "v2" means by a URL, then you have no business calling them two different "versions" when you actually only have ONE endpoint.

This is extremely misleading and almost enough for me to consider abandoning this "API" since you keep messing around with existing endpoints.

@jimaek / @UnbounDev , please advise.

@mstenta
Copy link

mstenta commented Jan 27, 2016

I agree 100% with @MarkCarver on this one. I have hundreds of sites using the Bootstrap theme, and they are all currently broken because of this. This is the second time this has happened.

Can you please revert the commit until a proper solution is figured out?

MartinKolarik added a commit to MartinKolarik/api-sync that referenced this issue Jan 27, 2016
@mstenta
Copy link

mstenta commented Jan 27, 2016

Thanks for reverting @MartinKolarik - does this go live immediately, or is there a deployment process? Just want to know if I need to start putting out fires, or if they will go out by themselves...

@MartinKolarik
Copy link
Member

@mstenta If I could I'd have reverted it right away, but we need to wait for @jimaek to merge it. Expect a couple of hours as he's in UTC+1 timezone.

@markhalliwell
Copy link

At first I thought this just affected the UI (which is still a pretty big bug), but it turns out that it actually causes sites to become completely broken after its Drupal cache has been cleared. Example: https://dreditor.org/

dreditor

I understand the timezone thing, here's hoping the revert can be deployed ASAP.

@MartinKolarik
Copy link
Member

Sorry for any problems that this caused. The change was reverted earlier today and the API now works as before.

@MarkCarver

but it turns out that it actually causes sites to become completely broken after its Drupal cache

To be honest, that seems like something that could be improved on your end. Once the user selects a theme, you can store the needed URLs in some kind of persistent storage. Even if the API went down, it shouldn't affect anyone except people trying to change the theme at that moment. Correct me if there's actually a good reason why you can't do this.

Once again, this was clearly a problem on our end, and we'll do our best to prevent this from happening in the future. Just saying that you also have the ability to make your theme more reliable.

@markhalliwell
Copy link

Once the user selects a theme, you can store the needed URLs in some kind of persistent storage. Even if the API went down, it shouldn't affect anyone except people trying to change the theme at that moment.

Sure. I can certainly take some responsibility here on that front. I'm not entirely sure how I'll do it though given how convoluted Drupal's theming system is. I'll likely have to manually build in even more redundancies.

I get this isn't a perfect world and things break. However, the "versioning" jsDelivr thinks it is providing needs some very serious refactoring as I have said in jsdelivr/api-sync#48 (comment).

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

5 participants