Skip to content

Fix library's imported name using requirejs AMD loader - #6440

Merged
archmoj merged 7 commits into
masterfrom
false-umdNamedDefine
Jan 18, 2023
Merged

Fix library's imported name using requirejs AMD loader#6440
archmoj merged 7 commits into
masterfrom
false-umdNamedDefine

Conversation

@archmoj

@archmoj archmoj commented Jan 17, 2023

Copy link
Copy Markdown
Contributor

Potential fix discovered by @alexcjohnson for AMD require in the docs

@plotly/plotly_js

@archmoj archmoj added bug something broken status: reviewable labels Jan 17, 2023
@archmoj archmoj changed the title set umdNamedDefine webpack config option to false set umdNamedDefine webpack config option to false Jan 17, 2023
@alexcjohnson

Copy link
Copy Markdown
Collaborator

Sure enough, in the unminified bundle I see define([], factory); like we had previously with browserify rather than the define('Plotly', [], factory); it had in the previous webpack build. So I expect this will work, but how hard would it be to test it in the actual docs pipeline? @nicolaskruchten ?

After define there are still some different code paths between the two headers, but the only other one I'm aware of that we care about is setting window.Plotly and they both do that - browserify by explicitly looking for window, webpack by setting self.Plotly and assuming self===window.

BTW, the link above was from the artifacts tab of the circleci job, if I go to the "Show URLs to Build Files" step instead the links (like https://176356-45646037-gh.circle-artifacts.com/0/dist/plotly.js) don't work.

@nicolaskruchten

Copy link
Copy Markdown
Contributor

you can just test it with a requirejs snippet I think...

<script type="text/javascript" src="proxy.php?url=https%3A%2F%2Fcdnjs.cloudflare.com%2Fajax%2Flibs%2Frequire.js%2F2.3.2%2Frequire.js"></script> to load it, then

<script type="text/javascript">
window.PlotlyConfig = {MathJaxConfig: 'local'};
if (window.MathJax && window.MathJax.Hub && window.MathJax.Hub.Config) {window.MathJax.Hub.Config({SVG: {font: "STIX-Web"}});}
if (typeof require !== 'undefined') {
require.undef("plotly");
requirejs.config({
    paths: {
        'plotly': ['https://cdn.plot.ly/plotly-2.16.1.min']
    }
});
require(['plotly'], function(Plotly) {
    window._Plotly = Plotly;
});
}
</script>

to configure

then try something like require(["plotly"], function(Plotly) { Plotly.newPlot() } to see if it works? Maybe throw together a codepen with that and see? you'd have to point the configuration URL to a build artefact I think.

@archmoj

archmoj commented Jan 18, 2023

Copy link
Copy Markdown
Contributor Author

Sure enough, in the unminified bundle I see define([], factory); like we had previously with browserify rather than the define('Plotly', [], factory); it had in the previous webpack build. So I expect this will work...

For reference here is the diff between before and after applying 16f1d1f.
Screenshot from 2023-01-18 10-07-59

@nicolaskruchten

Copy link
Copy Markdown
Contributor

ok so here is a Pen with 2.16 https://codepen.io/nicolaskruchten/pen/zYLPGgY

here is a broken one with 2.17 https://codepen.io/nicolaskruchten/pen/NWBwGKL

here is one with the artifact from this branch (I think!) https://codepen.io/nicolaskruchten/pen/BaPmoax

@nicolaskruchten

Copy link
Copy Markdown
Contributor

Generally it looks like it works?

@nicolaskruchten

Copy link
Copy Markdown
Contributor

(I just lifted some bits of code from https://plotly.com/python/bar-charts/ so I'm fairly confident that this is representative of what goes on in py-docs)

@alexcjohnson alexcjohnson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃 Excellent! Let's do it 🚀

Comment thread devtools/test_dashboard/server.js Outdated
@archmoj
archmoj force-pushed the false-umdNamedDefine branch from 2aa745d to 40cecb6 Compare January 18, 2023 20:59
@archmoj archmoj added the regression this used to work label Jan 18, 2023
archmoj and others added 2 commits January 18, 2023 16:35
Co-authored-by: Alex Johnson <alex@plot.ly>
@archmoj archmoj changed the title set umdNamedDefine webpack config option to false Fix library's imported name using requirejs AMD loader Jan 18, 2023
@archmoj
archmoj merged commit 360213e into master Jan 18, 2023
@archmoj
archmoj deleted the false-umdNamedDefine branch January 18, 2023 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug something broken regression this used to work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants