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

Render math with MathJax #12028

Merged
merged 8 commits into from
Aug 8, 2022
Merged

Render math with MathJax #12028

merged 8 commits into from
Aug 8, 2022

Conversation

miketheman
Copy link
Member

No description provided.

@miketheman miketheman requested a review from a team as a code owner August 6, 2022 22:41
@miketheman
Copy link
Member Author

I split into commits to make review easier (I hope). Each commit has more detailed descriptions.

I chose to use the CDN approach over trying to load an npm package and further expand the current packaged javascript, so as to keep the page loading fast and allow for async loading of the added code. A tradeoff is expanding the CSP rules to accommodate, at least until we come back to the frontend pipeline and potentially using more of webpack to split up the client code a little more.

@miketheman
Copy link
Member Author

Here's what it looks like with the SVG renderer, and using the Accessibility extensions to expand/collapse.

Screen.Recording.2022-08-06.at.6.47.32.PM.mov

Also loads the screen reader plugin in the background.

@di
Copy link
Member

di commented Aug 7, 2022

I think I'd rather just host the JS ourselves. We could just serve the JS bundle as a static asset similar to our other JS assets if we wanted to avoid mucking about with the frontend pipeline.

@miketheman
Copy link
Member Author

@di I tinkered with it and think I got it right in 8334ac9

I didn't vendor the entire mathjax package, only the files necessary to load and render tex-style and svg output - if we need more than that, we'll have to add those files.

Changed: I moved the loading script tag to only load on warehouse/templates/packaging/detail.html where it's expected to be used, so other parts of the site don't have to load it.

@miketheman
Copy link
Member Author

CodeQL doesn't like the inclusion of the compiled module - uncertain what the implications of that are.

The latest release includes the necessary changes to render the HTML
correctly for RST with Math directives.

Refs: https://github.com/pypa/readme_renderer/releases/tag/36.0

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Follows installation instructions from docs.
See https://docs.mathjax.org/en/latest/web/start.html#using-mathjax-from-a-content-delivery-network-cdn
Using the TeX+SVG variant to keep it lightweight.

Loads from a CDN with SRI hashes, and restricts the Content Security
Policy narrowly to prevent any other calls being made to anything
unrelated to the MathJax package.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
MathJax 3.2.2

Files required for `tex-svg.js` to load and execute

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Follows installation instructions from docs.
See https://docs.mathjax.org/en/latest/web/start.html#using-mathjax-from-a-content-delivery-network-cdn
Using the TeX+SVG variant to keep it lightweight.

Loads from a CDN with SRI hashes, and restricts the Content Security
Policy narrowly to prevent any other calls being made to anything
unrelated to the MathJax package.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
MathJax 3.2.2

Files required for `tex-svg.js` to load and execute

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
@miketheman miketheman force-pushed the miketheman/mathjax branch 2 times, most recently from c389b28 to aa2faad Compare August 8, 2022 11:54
@ewdurbin
Copy link
Member

ewdurbin commented Aug 8, 2022

I’m -1 on rendering this with JS client side as it kinda defeats the purpose of readme_renderer, which is to reduce the ability for user content to interact with PyPI sessions, which this opens us up to if some vuln exists with the library.

To address the request for math rendering to match GFM, I’d be more interested to see it implemented as part for cmark/cmarkgfm.

I haven’t looked but I suppose this is moot if it’s what GitHub is doing I’d trust the addition of the client side code more readily.

@ewdurbin
Copy link
Member

ewdurbin commented Aug 8, 2022

@miketheman
Copy link
Member Author

@ewdurbin Thanks. I've been digging through this, and following the current practices on GitHub.

This change is specifically for RST right now - GFM is not addressed (yet).

@ewdurbin
Copy link
Member

ewdurbin commented Aug 8, 2022

I’m not really opposed to loading the library from CDN given the integrity checks, is there a particular reason you flagged it for vendoring @di?

@miketheman
Copy link
Member Author

To address the request for math rendering to match GFM, I’d be more interested to see it implemented as part for cmark/cmarkgfm.

@ewdurbin GFM is tracked in pypa/readme_renderer#214

@di
Copy link
Member

di commented Aug 8, 2022

I’m not really opposed to loading the library from CDN given the integrity checks, is there a particular reason you flagged it for vendoring @di?

Just inherent distrust of third party CDNs and the JS ecosystem I guess 🙂. There's a lot that could be hosted on JSdeliver and it felt like overkill to poke so many holes in our CSP for it.

@di di merged commit 6dec6a6 into pypi:main Aug 8, 2022
di added a commit to di/warehouse that referenced this pull request Aug 8, 2022
di added a commit that referenced this pull request Aug 8, 2022
@miketheman miketheman deleted the miketheman/mathjax branch August 8, 2022 22:51
SamirPS pushed a commit to SamirPS/warehouse that referenced this pull request Aug 30, 2022
* chore(deps): upgrade readme-renderer

The latest release includes the necessary changes to render the HTML
correctly for RST with Math directives.

Refs: https://github.com/pypa/readme_renderer/releases/tag/36.0

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* feat: include mathjax

Follows installation instructions from docs.
See https://docs.mathjax.org/en/latest/web/start.html#using-mathjax-from-a-content-delivery-network-cdn
Using the TeX+SVG variant to keep it lightweight.

Loads from a CDN with SRI hashes, and restricts the Content Security
Policy narrowly to prevent any other calls being made to anything
unrelated to the MathJax package.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* Revert "feat: include mathjax"

This reverts commit 642ce88.

* feat: include mathjax as vendored

MathJax 3.2.2

Files required for `tex-svg.js` to load and execute

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* feat: include mathjax

Follows installation instructions from docs.
See https://docs.mathjax.org/en/latest/web/start.html#using-mathjax-from-a-content-delivery-network-cdn
Using the TeX+SVG variant to keep it lightweight.

Loads from a CDN with SRI hashes, and restricts the Content Security
Policy narrowly to prevent any other calls being made to anything
unrelated to the MathJax package.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* Revert "feat: include mathjax"

This reverts commit 642ce88.

* feat: include mathjax as vendored

MathJax 3.2.2

Files required for `tex-svg.js` to load and execute

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
SamirPS pushed a commit to SamirPS/warehouse that referenced this pull request Aug 30, 2022
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.

3 participants