Opened 3 years ago
Closed 3 years 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, GitHub, GitLab) | Commit: | a355a67151634944683c4f987630cd03f33f4417 |
Dependencies: | Stopgaps: |
Description (last modified by )
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 3 years ago by
- Branch set to u/paulmasson/three_js__fix_cdn_fallback
comment:2 Changed 3 years ago by
- 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
comment:3 follow-up: ↓ 8 Changed 3 years ago by
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: ↓ 10 Changed 3 years ago by
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?
comment:5 follow-up: ↓ 9 Changed 3 years ago by
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?
comment:6 Changed 3 years ago by
- Commit changed from 378512d2fb497c4297895ae9386afe862a26249b to 9a81aedc9a591d8ca64605ad2fab601d70609735
Branch pushed to git repo; I updated commit sha1. New commits:
9a81aed | Fix CDN fallback
|
comment:7 Changed 3 years ago by
- Description modified (diff)
comment:8 in reply to: ↑ 3 Changed 3 years ago by
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: ↓ 11 Changed 3 years ago by
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: ↓ 12 Changed 3 years ago by
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 3 years ago by
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 3 years ago by
- 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: ↓ 21 Changed 3 years ago by
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 3 years ago by
- Status changed from positive_review to needs_info
comment:15 Changed 3 years ago by
- Status changed from needs_info to needs_review
comment:16 Changed 3 years ago by
- Commit changed from 9a81aedc9a591d8ca64605ad2fab601d70609735 to a355a67151634944683c4f987630cd03f33f4417
Branch pushed to git repo; I updated commit sha1. New commits:
a355a67 | Better HTML output
|
comment:17 Changed 3 years ago by
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 3 years ago by
I currently waiting for my upgrade on CoCalc?. Somehow, I cannot build Sage there anymore…
comment:19 Changed 3 years ago by
- Cc slelievre added
comment:20 Changed 3 years ago by
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 3 years ago by
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 3 years ago by
- Status changed from needs_review to positive_review
comment:23 Changed 3 years ago by
- Keywords threejs added
comment:24 Changed 3 years ago by
- Branch changed from u/paulmasson/three_js__fix_cdn_fallback to a355a67151634944683c4f987630cd03f33f4417
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Fix CDN fallback