search

sagemath / sagecell

The Sage Cell Server---providing a way to embed Sage computations into any web page.
Other
203 stars 68 forks source link

Ensure globals are set/read in exactly one place #550

Closed siefkenj closed 2 years ago

siefkenj commented 2 years ago

This PR does two things:

  1. It ensures that window.sagecell (and other globals) are set and read from exactly one place (in the start of main.js). A sagecell.js is now imported into files that used to rely on window.sagecell being defined. This means the files can be moved around/globals can be renamed without affecting functionality.
  2. domReady is now explicitly called when needed. It was previously loaded as domReady! which is a requirejs directive to tell requirejs not to execute the function until domReady has completed. There is no webpack version of this functionality. So, domReady is not explicitly called where needed.

For 2., I have tested the code, but the code also seems to work if domReady is excluded entirely. I would have expected it to fail, but I believe the asynchronous creation of makeSagecell allows the time for the dom to be loaded.

siefkenj commented 2 years ago

I am not going to be over picky given that you are doing all the work and it is going in the right direction, but it would be nice to have more certainty about domReady - is the new version indeed correct, or it just happens to work on a particular nice computer which does things fast enough?

I wrapped all entry points that access the dom in a call to domReady. I searched for a way to delay the loading of the dom in debugging mode, but I couldn't find any such thing. I believe the new version is correct, but coding can be hard...

novoselt commented 2 years ago

We'll do the usual field testing then - release it to public servers and see if there are any complaints and reproducible errors, so we have something to debug.

siefkenj commented 2 years ago

The move of domReady inside make() creates a pretty big diff, but hopefully you like it!