Opened 8 months ago

Closed 8 months ago

#27575 closed defect (fixed)

Three.js: Fix CDN Fallback

Reported by: paulmasson Owned by:
Priority: major Milestone: sage-8.8
Component: graphics Keywords: threejs
Cc: egourgoulhon, novoselt, slelievre Merged in:
Authors: Paul Masson Reviewers: Julian Rüth
Report Upstream: N/A Work issues:
Branch: a355a67 (Commits) Commit: a355a67151634944683c4f987630cd03f33f4417
Dependencies: Stopgaps:

Description (last modified by 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.

Change History (24)

comment:1 Changed 8 months ago by paulmasson

  • Branch set to u/paulmasson/three_js__fix_cdn_fallback

comment:2 Changed 8 months ago by paulmasson

  • Authors set to Paul Masson
  • Cc egourgoulhon novoselt added
  • Commit set to 378512d2fb497c4297895ae9386afe862a26249b
  • Component changed from PLEASE CHANGE to graphics
  • Description modified (diff)
  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to defect

New commits:

378512dFix CDN fallback

comment:3 follow-up: Changed 8 months ago by 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?

comment:4 follow-up: Changed 8 months ago by 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?

Last edited 8 months ago by saraedum (previous) (diff)

comment:5 follow-up: Changed 8 months ago by 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?

Last edited 8 months ago by saraedum (previous) (diff)

comment:6 Changed 8 months ago by git

  • Commit changed from 378512d2fb497c4297895ae9386afe862a26249b to 9a81aedc9a591d8ca64605ad2fab601d70609735

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

9a81aedFix CDN fallback

comment:7 Changed 8 months ago by paulmasson

  • Description modified (diff)

comment:8 in reply to: ↑ 3 Changed 8 months ago by 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.

comment:9 in reply to: ↑ 5 ; follow-up: Changed 8 months ago by 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.

comment:10 in reply to: ↑ 4 ; follow-up: Changed 8 months ago by 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.

comment:11 in reply to: ↑ 9 Changed 8 months ago by 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.

comment:12 in reply to: ↑ 10 Changed 8 months ago by saraedum

  • Reviewers set to Julian Rüth
  • Status changed from needs_review to positive_review

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.

comment:13 follow-up: Changed 8 months ago by 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.

comment:14 Changed 8 months ago by saraedum

  • Status changed from positive_review to needs_info

comment:15 Changed 8 months ago by saraedum

  • Status changed from needs_info to needs_review

comment:16 Changed 8 months ago by git

  • Commit changed from 9a81aedc9a591d8ca64605ad2fab601d70609735 to a355a67151634944683c4f987630cd03f33f4417

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

a355a67Better HTML output

comment:17 Changed 8 months ago by 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.

comment:18 Changed 8 months ago by saraedum

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

comment:19 Changed 8 months ago by saraedum

  • Cc slelievre added

comment:20 Changed 8 months ago by saraedum

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

comment:21 in reply to: ↑ 13 Changed 8 months ago by 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!

comment:22 Changed 8 months ago by saraedum

  • Status changed from needs_review to positive_review

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.

comment:23 Changed 8 months ago by embray

  • Keywords threejs added

comment:24 Changed 8 months ago by vbraun

  • Branch changed from u/paulmasson/three_js__fix_cdn_fallback to a355a67151634944683c4f987630cd03f33f4417
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.