Closed siefkenj closed 2 years ago
I strongly oppose downloading new versions instead of using those which are shipped with Sage. How many people are working on this code and how difficult it is to get a working installation of Sage for them with some help? Not that their needs are not important, but certainly it is a much lower priority than eliminating potential incompatibilities in the Sage version of Python files and SageMathCell version of JavaScript files. It also makes it more difficult to roll back to versions that used to work - if you want to play with an older version of Sage and match it with older version of JavaScript, it happens automatically at the moment.
- It appears
jquery.ui.touch-punch
is never actually used. Can we get rid of it?
Yes, it was disabled for a while now and nobody complained.
- The
npm
version ofJSmol
injects thejsmol.min.js
file into the document source upon import rather than include the source directly. I assume this is OK?
I don't quite understand the difference, can you please elaborate?
- We can reduce the filesize significantly by including fewer
moment.js
locales. Are they all needed? Where ismoment.js
even used?
No idea!
- We can further reduce the filesize by including fewer
jquery-ui
components. Do you know which ones are needed?
No!
But I am not convinced that there is a bit point in striving for the smallest possible size - your work already makes a drastic difference and overall size does not seem to be a bottleneck to me. Of course it does not hurt to improve further, but how can you actually figure out which files are used and which are not? Most likely they are included because Jupyter included them.
I also wonder if there is a point in striving for consistent indentation, if it changes back and forth. I'd much rather go a bit against the consistency, but see what the actual changes are.
What is package-lock.json
?
With the new changes, will we still have a private jQuery in case the web page loads a different version?
package-lock.json
records the exact version of all dependencies installed. It enables reproducible builds with npm install
(only the version of immediate dependencies are listed in package.json
).
A private version of jQuery is still being used, thought it is also assigned to the global variable $
and jQuery
. Libraries like jquery-ui
use the global variable. All sagecell files, however, import the private version directly and ignore the global variable.
If you're happy with the files size, that's fine with me!
I don't quite understand the difference, can you please elaborate?
The npm version of jsmol adds a <script src="http://jmol.sourceforge.net/jmol/JSmol.min.js"></script>
tag to the page instead of pulling in the code directly.
I also wonder if there is a point in striving for consistent indentation, if it changes back and forth. I'd much rather go a bit against the consistency, but see what the actual changes are.
It shouldn't be going back and forth that much after the transition to modules is finalized. I am a strong advocate of automatic formatting (it also makes code much easier to write when you know the formatting will be fixed for you without effort!)
I strongly oppose downloading new versions instead of using those which are shipped with Sage.
I can revert the changes to the Makefile. However, I haven't been able to get a working version of Sage compiled and have been relying on the ZIP file you attached. It basically means that I cannot run make
because the Makefile deletes build
...I have to say, I almost gave up on this project multiple times because of the build difficulties.
Would you be open to having a flag that specifies that deps should be downloaded but which defaults to copying from the Sage directory?
Would you be open to having a flag that specifies that deps should be downloaded but which defaults to copying from the Sage directory?
Of course! I have nothing against new features, but prefer to keep current working ones instead of potentially breaking them.
Troubles building SageMath itself are a bit orthogonal and in general should be easier to resolve, given the size of community... But in any case SageMathCell makes no sense without it and the primary goal is to have a reliable system for maintaining public servers. Without any false modestly I claim that I greatly improved the situation since the time I picked up, even though it is not perfect ;-)
Back to building Sage - when you make changes without it, you don't catch all the issues that happen with Makefile. I have no problem trying to fix them myself, but in general it is still highly preferable that one builds their own Sage when working on SageMathCell. So no need to go away from it as a default.
- A private version of jQuery is still being used, thought it is also assigned to the global variable
$
andjQuery
. Libraries likejquery-ui
use the global variable. All sagecell files, however, import the private version directly and ignore the global variable.
To clarify - if global variables are already defined, we do NOT redefine them, correct? My understanding is that this was exactly the setup with RequireJS and it was introduced because of people having issues. So it is important that they are still free to use their own jQuery and configure them as they see fit.
Thank you for addressing other issues too. It would be preferable to use Sage-shipped jsmol, for the same potential compatibility issues (and Sage may have a patched version) and for the sake of being internet-independent. Not a huge priority in our case, for sure, but on my last SageDays in ~2016 we actually had a very bad internet connection and it was important to be able to work without it!
To clarify - if global variables are already defined, we do NOT redefine them, correct? My understanding is that this was exactly the setup with RequireJS and it was introduced because of people having issues. So it is important that they are still free to use their own jQuery and configure them as they see fit.
I looked into this and it turns out global variables are never set, so no interference with user-installed jQuery. The Webpack plugin actually searches for all references to window.$
and window.jQuery
, etc and replaces them with our imported jQuery. Therefore, not even jquery-ui will leak to the outside world!
I have restored the old makefile behavior and added an environment variable to trigger the github-fetching behavior. Please test :-)
Are we supposed now to run make
and then make static/embedded_sagecell.js
??? Shouldn't the first one be sufficient for everything? Both give errors ;-)
novoselt@mncds:~/sagecell$ ../sage/sage -sh -c "make -B"
if git submodule status | grep -q ^[+-]; then git submodule update --init > /dev/null; fi
npm install
npm WARN read-shrinkwrap This version of npm is compatible with lockfileVersion@1, but package-lock.json was generated for lockfileVersion@2. I'll try to do my best with it!
added 424 packages from 1217 contributors and audited 379 packages in 20.534s
35 packages are looking for funding
run `npm fund` for details
found 2 moderate severity vulnerabilities
run `npm audit fix` to fix them, or `npm audit` for details
ifeq (,)
/bin/sh: 1: Syntax error: word unexpected (expecting ")")
Makefile:21: recipe for target 'build' failed
make: *** [build] Error 2
novoselt@mncds:~/sagecell$ make static/embedded_sagecell.js
npm run build
> embedded_sagecell.js@1.0.0 build /home/novoselt/sagecell
> webpack --mode production
assets by status 1.13 MiB [cached] 1 asset
runtime modules 1.02 KiB 6 modules
modules by path ./node_modules/ 2.26 MiB 299 modules
modules by path ./js/ 124 KiB
./js/main.js 4.33 KiB [built] [code generated]
./js/sagecell.js 302 bytes [built] [code generated]
./js/cell.js 14.1 KiB [built] [code generated]
./js/gaq.js 218 bytes [built] [code generated]
+ 11 modules
modules by path ./build/vendor/ 139 KiB
modules by path ./build/vendor/base/js/*.js 44.2 KiB 3 modules
modules by path ./build/vendor/services/kernels/*.js 52.5 KiB 3 modules
modules by path ./build/vendor/*.js 42.4 KiB 2 modules
ERROR in ./js/css.js
Module not found: Error: Can't resolve 'colorpicker.css' in '/home/novoselt/sagecell/js'
@ ./js/cell.js
@ ./js/main.js
ERROR in ./js/css.js
Module not found: Error: Can't resolve 'fontawesome.css' in '/home/novoselt/sagecell/js'
@ ./js/cell.js
@ ./js/main.js
ERROR in ./js/css.js
Module not found: Error: Can't resolve 'sagecell.css' in '/home/novoselt/sagecell/js'
@ ./js/cell.js
@ ./js/main.js
3 errors have detailed information that is not shown.
Use 'stats.errorDetails: true' resp. '--stats-error-details' to show it.
webpack 5.69.1 compiled with 3 errors in 42168 ms
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! embedded_sagecell.js@1.0.0 build: `webpack --mode production`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the embedded_sagecell.js@1.0.0 build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
npm ERR! A complete log of this run can be found in:
npm ERR! /home/novoselt/.npm/_logs/2022-02-26T04_47_47_284Z-debug.log
Makefile:45: recipe for target 'static/embedded_sagecell.js' failed
make: *** [static/embedded_sagecell.js] Error 1
Just make
is sufficient.
Sorry, apparently I don't understand Makefile
s very well...I have made a new PR that should fix the problem. Apparently make build
was not running because make
thought it had already run and there were no changes.
It got better and I manage one build, but then starting afresh I get ''' make: ** No rule to make target 'build/vendor/', needed by 'build'. Stop. ''' Removing this dependency seems to fix it, so I'll do it.
This PR switches the build system to Webpack (a more modern and better-supported build system than RequireJS). Webpack supports RequireJS syntax via the
@babel/plugin-transform-modules-amd
plugin.Major changes:
npm
are installed vianpm
and used directly.vendor
deps not available vianpm
are downloaded and copied to thebuild/vendor
directory via thefetch_vendor_js.mjs
script. Most files are taken directly from the Jupyter notebook's Github repo.static/
directory. The browser can use these while debugging to tell you the original source file that, e.g., a Javascript error occurred in.embedded_sagecell.js.LICENSE.txt
file is copied tostatic/
. This is autogenerated by Webpack and contains a copy of the license from each module that's included vianpm install
.The majority of the build steps are moved from
Makefile
to build scripts run vianode
. A big plus is that you don't need to install/build Sage to work on the JS.As for 2, you may or may not like it. Since the files are copied directly from github, they may be newer than your currently installed version of Sage (I think the benefits of being able to work on this project without downloading hundreds of MB of source is worth it, though.). If it's an issue, a "use vendor sources from sage directory" option could be added to the Makefile.
I converted most
require
statements to ES6 import statements (which get compiled by Webpack into ES5), but I didn't fully convert everything since that will result in a huge diff and I wanted the changes to be clear. (Basically, all thedefine(function(){ ... })
parts will be replaced with...; export ...
which will cause a lot of indentation changes.Webpack also has a ton of options via the
babel
plugin for cross-compiling. If needed we can enable the option to cross-compile for older web browsers.Questions
jquery.ui.touch-punch
is never actually used. Can we get rid of it?npm
version ofJSmol
injects thejsmol.min.js
file into the document source upon import rather than include the source directly. I assume this is OK?moment.js
locales. Are they all needed? Where ismoment.js
even used?jquery-ui
components. Do you know which ones are needed?If you run
you will get a list of every imported file and its size. There might be some other stuff that can be trimmed.