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

`extra_*` settings do not support pointing to CDNs #1336

Closed
Frederick-S opened this Issue Nov 1, 2017 · 9 comments

Comments

Projects
None yet
3 participants
@Frederick-S

Frederick-S commented Nov 1, 2017

This error occurs when I run mkdocs serve command. And the output shows:

INFO    -  Building documentation...
INFO    -  Cleaning site directory

Bad path: https:\\cdnjs.cloudflare.com\ajax\libs\mathjax\2.7.1\mathjax.js?config=tex-ams-mml_htmlormml

The output shows mkdocs converts url to lower case, but mathjax cdn is case sensitive, so the converted url is not valid.

Here is my extra_javascript setting: extra_javascript: ['https://cdnjs.cloudflare.com/ajax/libs/mathjax/2.7.1/MathJax.js?config=TeX-AMS-MML_HTMLorMML', 'mathjaxconfig.js'].

And additional information:

OS: Windows 10
Python: 3.6.1
mkdocs: 0.17.0

@waylan

This comment has been minimized.

Member

waylan commented Nov 1, 2017

I believe that the extra_javascript setting assumes that all provided paths are for local files in your docs_dir. You are getting the "bad path" error because the provided string is not a valid path to a local file.

There is currently no support for listing external resources in the extra_* settings. And its not clear to me if there should be. I'm considering this as a feature request.

As a workaround, you would need to host the MathJax lib locally (include a copy in the docs_dir) or use a theme custom_dir to include the remote MathJax lib in the libs block of the template (as described here).

@waylan waylan changed the title from "Bad path" error occurred on Windows 10 to `extra_*` settings do not support pointing to CDNs Nov 1, 2017

@facelessuser

This comment has been minimized.

Contributor

facelessuser commented Nov 1, 2017

I'm pretty sure this used to work and was broken at some point. I'm not sure why this would be specific disallowed and required to be a local file. This just doesn't seem practical to how people are actually found to try and use this and would expect this to work.

@waylan

This comment has been minimized.

Member

waylan commented Nov 1, 2017

I'm pretty sure this used to work and was broken at some point.

Did it? I'm not sure one way or the other. I just know it doesn't work now. If it did work, then that is a regression and this should be a bug.

@facelessuser

This comment has been minimized.

Contributor

facelessuser commented Nov 1, 2017

I, at one point, used to do exactly what is described in this issue to apply MathJax. I do things a bit differently now as I have reasons to do so, but due to how I used it in the past, I would try to do this in the future if I needed to pull in a library from a CDN.

So I don't know when it broke, but I would have thought this breakage a bug.

@waylan

This comment has been minimized.

Member

waylan commented Nov 1, 2017

It appears that this line (list(os.path.normcase(path) for path in value)) was introduced in #1308. In fact, I expect that is where the "bad path" error is coming from. This is definitely a regression and a bug.

@facelessuser

This comment has been minimized.

Contributor

facelessuser commented Nov 1, 2017

My personal opinion on MathJax is that it is a weighty library to include on every page even if it doesn't use math. So I personally shy away from including via extra.

I prefer to append the library to the end of that file I am including math in: https://raw.githubusercontent.com/facelessuser/pymdown-extensions/master/docs/src/markdown/_snippets/mathjax.md. So I create a markdown partial that I include at the bottom of other markdown pages with a Markdown extension.

You can just copy something similar at the body of the pages that have math, or use a Markdown extension like this (which I am the author of) or I assume this (which uses a less whimsical syntax if that is something that bothers you). Also you could probably accomplish this by customizing your theme with a template override, or even use the new MkDocs plugin system.

But either way, it will be nice to have this issue fixed.

@waylan

This comment has been minimized.

Member

waylan commented Nov 1, 2017

I just remembered why that change was made. In post_validation we are issuing a warning for any CSS/JavaScript files which are not listed as we transition the behavior to not autofill. However, to do a "contains" check, the paths need to be normalized as the comparison is a dumb string comparison, not a comparison of (url or path) objects.

I wonder if this would suffice, or if we need something more sophisticated:

list(os.path.normcase(path) for path in value if not path.startswith('http'))

As a reminder, in the next major release, this entire thing will be removed and the the extra settings will simply be validated as being a list.

@facelessuser

This comment has been minimized.

Contributor

facelessuser commented Nov 1, 2017

I think if it passes through URLs, people will be fine with it. People just want their CDN links to work 🙂 .

waylan added a commit to waylan/mkdocs that referenced this issue Nov 1, 2017

Correct `extra_*` config setting regressions.
The warning for missing files should only be issued if the config
setting is empty. The warning only exists because we disabled
autopopulating of the settings. Autopopulating only used to run
when the setting was empty so the warning is not nessecary if
the setting has been populated.

Given the above, no string comparisons need to be made as we
only need to check if any files exist when empty. Therefore, we
don't actually need to normalize paths. As we are no longer
modifying users' strings, they can again put any string in their
config, such as URLs of CDNs.

Fixes mkdocs#1335 & mkdocs#1336.
@waylan

This comment has been minimized.

Member

waylan commented Nov 1, 2017

Added a fix in #1337. Thanks to #1335, I realized we don't need to do any string comparisons. Therefore, we again have no modification of the setting.

waylan added a commit that referenced this issue Nov 1, 2017

Correct `extra_*` config setting regressions.
The warning for missing files should only be issued if the config
setting is empty. The warning only exists because we disabled
autopopulating of the settings. Autopopulating only used to run
when the setting was empty so the warning is not nessecary if
the setting has been populated.

Given the above, no string comparisons need to be made as we
only need to check if any files exist when empty. Therefore, we
don't actually need to normalize paths. As we are no longer
modifying users' strings, they can again put any string in their
config, such as URLs of CDNs.

Fixes #1335 & #1336.

@waylan waylan closed this Nov 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment