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

Use try...catch block with underscore.js template. #5984

Merged
merged 6 commits into from Jul 31, 2019

Conversation

@dojutsu-user
Copy link
Member

@dojutsu-user dojutsu-user commented Jul 24, 2019

#5980 doesn't fixes #5978
Turns out that the problem was not of variable declaration.
But the error was due to syntax changes in underscore.js templating engine.

See: https://stackoverflow.com/a/25881231/8601393

@dojutsu-user dojutsu-user added this to In progress in In-doc search UI via automation Jul 24, 2019
@dojutsu-user dojutsu-user requested a review from Jul 24, 2019
Copy link
Contributor

@davidfischer davidfischer left a comment

This looks fine, but I want a bit more details in the PR and in the comments.

Loading

// because of change of syntax in new versions.
// See: https://stackoverflow.com/a/25881231/8601393
try {
// this is the old syntax
Copy link
Contributor

@davidfischer davidfischer Jul 25, 2019

Choose a reason for hiding this comment

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

Let's be a little more specific in the comment here. Sphinx bundles Underscore.js v1.3.1 (and has for many years) and it looks like this syntax changed in Underscore v1.7.0 (not exactly how semver is supposed to work...). So Sphinx should always use the old syntax and almost never the new syntax. How was this throwing a syntax error exactly? Were people somehow bundling newer versions of Underscore?

Loading

Copy link
Contributor

@davidfischer davidfischer Jul 25, 2019

Choose a reason for hiding this comment

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

Ok, I figured it out. Sphinx has bundled Underscore.js v1.3.1 as I mentioned. However, doc builds before ~2019 used centrally located versions of jQuery and Underscore.js (due to readthedocs/readthedocs-sphinx-ext#52) as an optimization. That centrally located one uses Underscore.js v1.7.0. This optimization was removed precisely because it caused version mismatch issues like this! However, old doc builds still use this optimization.

Loading

Copy link
Contributor

@davidfischer davidfischer left a comment

I made one comment suggestion for clarity. Otherwise, this is good.

Loading

readthedocs/core/static-src/core/js/doc-embed/search.js Outdated Show resolved Hide resolved
Loading
Co-Authored-By: David Fischer <djfische@gmail.com>
@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Jul 25, 2019

Is it okay that we're assuming that underscore is loaded by the page? Is underscore a new implicit dependency with this change?

The only case I can think of where a user does not have underscore.js is if they have a heavily customized theme -- which is more of an issue on our commercial hosting.

Loading

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Jul 25, 2019

@agjohnson
I think that it is okay to assume this.
I am not sure of this, but I believe sphinx adds underscore.js in every docs.
I have one example -- https://docs.signalfx.com/en/latest/
This is a heavily customised theme and it does includes underscore.js.

Loading

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Jul 26, 2019

My point was I thought we needed to verify this, we've postulated about it a good deal. I inspected our commercial hosting, where themes are more heavily customized, and the only projects that did not have underscore.js were mkdocs projects. Sphinx adds these files if they are referenced, but a heavily customized theme might not trigger this reference. An implicit dependency seems okay here. 👍

Loading

Copy link
Member

@ericholscher ericholscher left a comment

👍 once the conflict is resolved.

Loading

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Jul 26, 2019

@ericholscher
This is mergeable now.

Loading

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Jul 26, 2019

The only case I can think of where a user does not have underscore.js is if they have a heavily customized theme -- which is more of an issue on our commercial hosting.

While it is possible, Sphinx search doesn't work without Underscore.js. So if they've customized so heavily that they've removed Underscore, they've likely also removed search.

Loading

@ericholscher ericholscher merged commit a7cd4e5 into readthedocs:master Jul 31, 2019
1 check passed
Loading
@dojutsu-user dojutsu-user deleted the fix-js-file-again branch Jul 31, 2019
@dojutsu-user dojutsu-user moved this from In progress to Done in In-doc search UI Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Linked issues

Successfully merging this pull request may close these issues.

4 participants