search

sagemath / sagecell

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

Vendor prefix all external JS deps #547

Closed siefkenj closed 2 years ago

siefkenj commented 2 years ago

Copy all external JavaScript into build/vendor instead of directly into build/. This should make it easier to separate internal vs external dependencies during development.

siefkenj commented 2 years ago

Since I still don't have a working sage build, I cannot test the make script, so hopefully you can test it for me. It works with the build.zip file you provided.

novoselt commented 2 years ago

I get adding vendor prefix, but could you please explain why a bunch of new named modules were added and why quotes were removed in some cases?

siefkenj commented 2 years ago

I get adding vendor prefix, but could you please explain why a bunch of new named modules were added and why quotes were removed in some cases?

Oh, sorry! I used Prettier to format the file. It normalizes all quotes to ". I can revert them if you like.

There actually weren't any new deps added. r.js would search the local directory for imported files not listed in the deps. After moving them to vendor, these imports failed, so I added them explicitly. Now the deps list + non-vendor js/html files should be the only things needed to build sagecell. (Currently a ton of uneeded files are copied into vendor by the Makefile. I am looking at trimming that down.)

novoselt commented 2 years ago

Almost everything is coming from cp -a $(SAGE_VENV)/lib/python3.9/site-packages/notebook/static the rest of dependencies are individual files. If we are to do the separation, wouldn't it be more natural then to have a separate path for this directory and another common one for the rest?

Regarding quotes I am confused by

        jquery: "vendor/components/jquery/jquery.min",
        "jquery-ui": "vendor/components/jquery-ui/jquery-ui.min",

I get that one can use single or double and perhaps it is prettier to use the same quotes. But why jquery has no quotes at all anymore, while jquery-ui does have them???

siefkenj commented 2 years ago

Almost everything is coming from cp -a $(SAGE_VENV)/lib/python3.9/site-packages/notebook/static the rest of dependencies are individual files. If we are to do the separation, wouldn't it be more natural then to have a separate path for this directory and another common one for the rest?

Conceptually that works, but do you think this would be helpful? I think the most important thing is that devs know what code they are responsible for (i.e., they know to completely ignore the vendor dir).

I get that one can use single or double and perhaps it is prettier to use the same quotes. But why jquery has no quotes at all anymore, while jquery-ui does have them???

Javascript objects do not require quoted strings unless the key contains a non-letter/non-number character. Usually keys are not quoted unless they need to be.

novoselt commented 2 years ago

Ah! Thank you for your patience in explaining JS basics ;-)

Regarding knowing what developers are responsible for - they should completely ignore the current build directory, all SageMathCell JavaScript code is contained in js directory. build is used to pile up everything together and build a single file, which is then copied into static. So if the point is clear separation of our code vs dependencies it seems to me that it is already achieved, although apparently it is not clearly explained. Perhaps instead of moving files around we should add something to the documentation?

siefkenj commented 2 years ago

Perhaps instead of moving files around we should add something to the documentation?

I think the files should still be put in vendor. The current build system is pretty unusual. Typically non-vendor files are not copied before a build. Instead the JS compiler works with the files in the js directory directly and outputs the result in a build directory. My hope is to unwind the Makefile a bit so that the JS can be build this way. But, I wanted to do incremental steps rather than presenting you with a big diff.

There are other benefits to the vendor/ approach

  1. It revealed unlisted dependencies (the ones you were wondering about earlier). 1b. It prevents people from accidentally using a dep just because it happens to be in the build directory. If they want to use a dep, they need to explicitly specify it in build.js.
  2. It more closely follows the node_modules approach of npm. Half of the deps (e.g. jQuery, undescore and friends) can be grabbed by running npm install jquery underscore. Only the sage-specfic deps would need to be copied. In this way, the vendor/ folder would act like the sage-specific node_modules folder.
novoselt commented 2 years ago

Makes sense - thank you once again for your patient explanations!

siefkenj commented 2 years ago

No problem!

My next plan is to sort out some of the global variable initialization which will pave the way to using ES imports instead of requirejs.