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 the latest thebe.js in our documentation #285

Closed
choldgraf opened this issue Oct 20, 2020 · 19 comments · Fixed by #472
Closed

Use the latest thebe.js in our documentation #285

choldgraf opened this issue Oct 20, 2020 · 19 comments · Fixed by #472
Projects

Comments

@choldgraf
Copy link
Member

choldgraf commented Oct 20, 2020

I think that it would be best for us to use the latest thebe.js package in our documentation, so that we can both test it locally in a more "realistic" setup, "version" our documentation in the same way that Python is versioned, and so that our docs can use features that may be available only in the "latest" release.

I think that the simplest way to do this would be:

  1. Generate a single thebe.js file that can be linked locally, move it to docs/_static/thebe.js

  2. Add docs/_static to the list of static paths for Sphinx

  3. Add the linked JS file via

    def setup(app):
        app.add_js_file("thebe.js")

The only challenge is that I have no idea how to build thebe.js locally in a way that could be linked in a local SSG. Does anyone have guidance on how / the best way to do this?

perhaps @minrk or @joelachance or @rowanc1 know how?

@moorepants
Copy link
Collaborator

Agreed, the versions of thebe used in the various docs versions should match. For example, if you want to write some explanation and demonstration about "Restart and Run All" you can't because it doesn't show up!

@joelachance
Copy link
Contributor

All of this seems sound to me-- I don't know about the Sphinx side of things, but we could write a custom git hook to run a script which would generate a thebe.js file inside the docs/_static/. Of course there's other solutions, but that's the first one that comes to mind.

Does this sound like a viable solution, @choldgraf?

@moorepants
Copy link
Collaborator

I'm not sure a git hook solves the issue. If you build the docs locally you'd like the version of thebe at that commit to be used to build the docs.

@joelachance
Copy link
Contributor

A git hook would merely run the script which would generate the build. It's a bonus, but not needed. The script could be an npm script living in package.json or something custom, but to your earlier point (a good one!), the script could generate versionedthebe.js builds, which could still be useful locally if we need to patch an older version (I don't know if we ever do this).
The 'build' step is pretty simple since we're already building Thebe, but the 'link in a local SSG' part I'm unsure about.

@moorepants
Copy link
Collaborator

I don't know enough about modern JS tech, but from the python side most sphinx conf.py file simply append the parent directory to sys.path. This ensures your docs are built with local version of the python lib. Python libs don't have to be built though. If thebe has to be built before it can be used, then I guess adding that build step to the sphinx make file would be needed. Sorry if you already know all this...

@choldgraf
Copy link
Member Author

choldgraf commented Oct 22, 2020

I feel like there are two options (both of which are fine with me):

  1. Add another make command to our docs makefule (like make js) that will build the JS file and put it in docs/_static. This is expected to be run by the developer and not checked in to the repository. If you want the latest package, you have to remember to run make js, or we could add a command like make html-all that would run make js first. In the online docs, we add this command to our conf.py file so that the JS is compiled when the docs are built.
  2. Add a pre-commit hook to re-build a JS file in docs/_static if anything in the package assets change. Commit this to the repository and link it locally in our docs. (we can use the pre-commit package for this)

@joelachance
Copy link
Contributor

Sorry if you already know all this...

@moorepants I didn't, thank you! I think you're right, we'll need that build step/make command.

This is expected to be run by the developer and not checked in to the repository.

@choldgraf I think this is a great idea, and would be good to have this exposed in an npm script too, just for ease of use. (i.e. npm run build:local --> make js)

@choldgraf , If you do want to check the build in (I know you said you didnt), I would lean towards using a pre-commit. With a pre-commit, we will always create a latest build with each commit, which would be convenient when checking for errors/bugs in PRs. With the conf.py, we'd need to run this each time we want to view changes. If we don't want to check the build in, either way seems great to me :)

Regardless of what we do, it's probably good to build again when we build the docs, just for the sake of being explicit (so a +1 for conf.py here). Thoughts?

@choldgraf
Copy link
Member Author

Perhaps a good way to get started is to write the npm run build:local script. Then we can figure out the best way to integrate that built JS with the documentation after trying it out? WDYT?

@joelachance
Copy link
Contributor

Seems reasonable!

I think that script would look something like: npm run build:prod && cp lib/index.js docs/__static/index.js, however...

The build:prod command generates a good number of files:
Screen Shot 2020-10-23 at 11 04 09 AM

which ones do we want to use?

@choldgraf
Copy link
Member Author

mmmm I have no idea :-) I assume the non-numbered index.js?

@joelachance
Copy link
Contributor

Based on the CDN url: https://unpkg.com/thebe@0.5.1/lib/index.js, I'm assuming that's true!

@choldgraf
Copy link
Member Author

my strategy with javascript is "try it and see if it breaks, if nothing obviously breaks then roll it to production :shipit:"

@joelachance
Copy link
Contributor

ha! That's mine too :)

@minrk
Copy link
Collaborator

minrk commented Oct 28, 2020

I think all the *.js files are needed. The .maps are only for debugging.

I would copy the whole lib folder so you don't need to worry about how webpack breaks up the bundle. It should use relative URLs to load multiple stages as appropriate, so you only need to have a script tag for index.js, but the rest maybe loaded asynchronously.

@joelachance
Copy link
Contributor

I was looking for any mentions of a 1.index.js or 2.index.js, but I can't find them anywhere. I also can't figure out why webpack is building them in the first place. Maybe I'm missing something, but regardless of relative urls, I would think we'd find those files mentioned somewhere to be loaded, since index.js is the only file served on the CDN (again, as far as I know).

What am I missing here?

@choldgraf
Copy link
Member Author

I took a pass at trying to implement this here:

#298

I got it working fine locally, but can't figure out how to get readthedocs to build the JS properly (it has npm 8.x by default and I think we need to somehow upgrade, but not sure how to do this without breaking things 🤦‍♂️

@joelachance
Copy link
Contributor

Is the readthedocs Node version coming from docs/environment.yml? Can we give that a version number?

@choldgraf
Copy link
Member Author

so when I used conda for the nodejs version, I got the right nodejs version and the JS built fine, but then for some reason sphinx_copybutton wasn't found and it makes me think all the pip-installed things aren't in the same environment, which confuses me

@joelachance
Copy link
Contributor

Shoot. I'm lost here, unfortunately. Anyone?

Also @minrk, still curious about if you have any insight:

I was looking for any mentions of a 1.index.js or 2.index.js, but I can't find them anywhere. I also can't figure out why webpack is building them in the first place. Maybe I'm missing something, but regardless of relative urls, I would think we'd find those files mentioned somewhere to be loaded, since index.js is the only file served on the CDN (again, as far as I know).

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

Successfully merging a pull request may close this issue.

4 participants