fix: prevent namespace pollution bug

[pankgeorg]

Description

fonsp/Pluto.jl#3306 and #5156

There is a small issue with the id generation; it should start with a letter.

Apart from that, the main issue here is referencing functions that get executed inside a different "module" (in julia terms). Pluto (and maybe also VSCode?) do not hoist the functions to the top level as you would expect, so referencing the created function from onload isn't working. The solution is simple; programmatically register the handler, and then appendChild the script to the DOM.

Attribution

• I am listed in .zenodo.json (see How to get acknowledged #3503)

Things to consider

• Does it work on log scales?
• Does it work in layouts?
• Does it work in recipes?
• Does it work with multiple series in one call?
• PR includes or updates tests?
• PR includes or updates documentation?

[JamesWrigley]

Thanks for looking into this, I had a go but was quite lost. For me this patch works the first time a plot is displayed in IJulia, the second time the plot isn't displayed and I see this in the console:

Uncaught SyntaxError: redeclaration of let plotlyloader
    <anonymous> scratch.ipynb line 162569 > injectedScript:1
    attachWidget index.es6.js:2640
    insertWidget index.es6.js:2543
    _insertOutput widget.js:579
    onModelChanged widget.js:305
    invokeSlot index.es6.js:555
    emit index.es6.js:513
    emit index.es6.js:112
    _onListChanged model.js:277
    invokeSlot index.es6.js:555
    emit index.es6.js:513
    emit index.es6.js:112
    push observablelist.js:139
    _add model.js:226
    add model.js:146
    _onIOPub widget.js:98
    _handleIOPub future.js:236

[fonsp]

You can fix that with IIFE, instead of:

let plotlyloader = window.document.createElement("script")
let src="https://requirejs.org/docs/release/$(PlotsBase._requirejs_version)/minified/require.js"
plotlyloader.addEventListener("load", plots_jl_plotly_$unique_tag);
plotlyloader.src = src
document.querySelector("#$unique_tag").appendChild(plotlyloader)

you do

;(() => {
let plotlyloader = window.document.createElement("script")
let src="https://requirejs.org/docs/release/$(PlotsBase._requirejs_version)/minified/require.js"
plotlyloader.addEventListener("load", plots_jl_plotly_$unique_tag);
plotlyloader.src = src
document.querySelector("#$unique_tag").appendChild(plotlyloader)
})()

[fonsp]

Or changing <script> to <script type="module"> is probably a better solution

[fonsp]

@JamesWrigley @BeastyBlacksmith Me and Panagiotis would be happy to have a call to go over the JS code in Plots.jl together? We're happy to help!

I think it would be nice to bring everything up to date, with MathJax 2 -> 3, plotlyjs 2 -> 3, removing requirejs and with modern JavaScript, imports and CDNs. That also makes future maintenance easier

[JamesWrigley]

I hate web dev so I'll politely decline 😅 I'm just a drive-by contributor trying to get Plotly and IJulia to work together. Removing requirejs sounds good though 👍 That was the original cause of the issue I was trying to fix since requirejs was previously bundled with Jupyter and Plots.jl was relying on that.

[BeastyBlacksmith]

@JamesWrigley @BeastyBlacksmith Me and Panagiotis would be happy to have a call to go over the JS code in Plots.jl together? We're happy to help!

I think it would be nice to bring everything up to date, with MathJax 2 -> 3, plotlyjs 2 -> 3, removing requirejs and with modern JavaScript, imports and CDNs. That also makes future maintenance easier

Thats a generous offer, thanks. For example next week tuesday somewhen between 9am and 1pm would work for me. We can also discuss the date on zulip if you prefer.

[fonsp]

Meeting tuesday 11 CEST :) https://meet.google.com/env-maxy-bzx if anyone wants to join

[pankgeorg]

Meeting tuesday 11 CEST :) https://meet.google.com/env-maxy-bzx if anyone wants to join

And in the meantime, I updated this PR with Fons' fix. The discussion stays relevant of course, as we probably shouldn't be doing these tricks anyway (with a script type="module" or similar approach. See you there!

[fonsp]

Should we work on the v2 branch or master? I also notice that my previous PR #4863 is not included in v2.

[t-bltg]

Should we work on the v2 branch or master? I also notice that my previous PR #4863 is not included in v2.

master for fixing fixing bugs, and v2 for fixing bugs + adding new features.

So in this case I guess you need two PRS one against master and another against v2.

[t-bltg]

I hate web dev so I'll politely decline 😅 I'm just a drive-by contributor trying to get Plotly and IJulia to work together.

😄, same here !

And in the meantime, I updated this PR with Fons' fix. The discussion stays relevant of course, as we probably shouldn't be doing these tricks anyway (with a script type="module" or similar approach.

I manually checked that the PR works so until someone else has a better proposal, I'm ok with this.

If we can get rid of requirejs I'm all in favor.

A PR against master is more important than v2 for the end users.

[pankgeorg]

I hate web dev so I'll politely decline 😅 I'm just a drive-by contributor trying to get Plotly and IJulia to work together.

😄, same here !

And in the meantime, I updated this PR with Fons' fix. The discussion stays relevant of course, as we probably shouldn't be doing these tricks anyway (with a script type="module" or similar approach.

I manually checked that the PR works so until someone else has a better proposal, I'm ok with this.

If we can get rid of requirejs I'm all in favor.

A PR against master is more important than v2 for the end users.

done at #5171!

[t-bltg]

Thanks ! Let's wait for the meeting results and then this can go in.