Three.js: Fix CDN Fallback

[paulmasson]

The Three.js viewer for notebooks already had a fallback to the online CDN when the local files are not loaded, which was silently failing. This small change fixes that.

Renders ​this discussion moot.

[paulmasson]

New commits:

​378512d

Fix CDN fallback

[dimpase]

The "mooted" discussion is about such a fallback should be on by default. Do I get it right that the fallback was simply broken, and the branch here fixes it, so that it silently works?

[saraedum]

Could you explain why this makes a difference? I.e., I don't understand why this change would make a difference to the browser.

Also, how does this compare to ​https://trac.sagemath.org/ticket/26434?

[saraedum]

I thought I had understood, but I think I don't. The online scripts don't seem to contain a '. So why did it not work before?

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​9a81aed

Fix CDN fallback

[paulmasson]

Replying to dimpase:

The "mooted" discussion is about such a fallback should be on by default. Do I get it right that the fallback was simply broken, and the branch here fixes it, so that it silently works?

It only silently works when the notebook is moved to a remote location. A locally served notebook should not have any problem finding the JS files and the the fallback will not be activated.

[paulmasson]

Replying to saraedum:

I thought I had understood, but I think I don't. The online scripts don't seem to contain a '. So why did it not work before?

A regular string in JS needs line continuation markers. The back ticks turn the string into a template literal, which is allowed to include new lines.

After reviewing the code in #22670 that added this fallback, I reverted to a normal JS string. That way older browsers and other environments should be supported.

[paulmasson]

Replying to saraedum:

Also, how does this compare to ​https://trac.sagemath.org/ticket/26434?

As a JS person, I would never think of copying an entire library into every file I create. While it is a possible solution, it just doesn't seem elegant. This fallback is intended for files shared on the web, which implies internet access to a CDN.

Please describe what problems you encountered at Luminy. Was there any lack of internet access? If this fallback had been working properly, then perhaps there would have been no issues.

[saraedum]

Replying to paulmasson:

Replying to saraedum:

I thought I had understood, but I think I don't. The online scripts don't seem to contain a '. So why did it not work before?

A regular string in JS needs line continuation markers. The back ticks turn the string into a template literal, which is allowed to include new lines.

Sure. That makes sense.

[saraedum]

Replying to paulmasson:

Replying to saraedum:

Also, how does this compare to ​https://trac.sagemath.org/ticket/26434?

As a JS person, I would never think of copying an entire library into every file I create. While it is a possible solution, it just doesn't seem elegant.

I agree that it's not elegant. But you probably wouldn't dynamically emit script tags, would you?

This fallback is intended for files shared on the web, which implies internet access to a CDN.

Personally, I really don't mind if this code contacts a CDN but one could argue that this could also run in a local webserver that is not meant to access the internet.

Please describe what problems you encountered at Luminy. Was there any lack of internet access? If this fallback had been working properly, then perhaps there would have been no issues.

There was no lack of internet access. I forget what the setup was unfortunately. Somehow this was served through an interface/web server that could not resolve the path to the local three.js assets. I assume that this fallback would have fixed the problem.

So, I'll set this to positive review as this definitely fixes an existing bug (currently, we are emitting JavaScript? code with a Syntax Error.) Whether this is a privacy issue in the first place can be discussed in a new issue imho. Feel free to set that one to critical if you have strong feelings about it.

[saraedum]

Ah, so the place where this does not work without the CDN is CoCalc? btw. Let me build Sage there to check that it is actually fixed with this.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​a355a67

Better HTML output

[paulmasson]

This final commit does not change functionality, but produces cleaner HTML output. The inserted script tags will now appear on separate lines. Sorry for any inconvenience this change might cause.

[saraedum]

I currently waiting for my upgrade on CoCalc?. Somehow, I cannot build Sage there anymore…

[saraedum]

slelievre: Could you test this on CoCalc? (or share one of your many upgrades with me so I can try myself?)

[egourgoulhon]

Replying to saraedum:

Ah, so the place where this does not work without the CDN is CoCalc? btw.

Another place where this does not work without the CDN is nbviewer.jupyter.org. With the code in this ticket branch, one can indeed get threejs rendering without having to add the option online=True, as ​this example demonstrates. Accordingly, I am +1 for a positive review!

[saraedum]

Building on CoCalc? failed somehow. Since it works on the nbviewer, I assume it also works on CoCalc?; at least it certainly won't make the situation worse.