Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Jan 18, 2023

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Jan 17, 2023

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
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
Contributor

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

<script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/require.js/2.3.2/require.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
Copy link
Contributor Author

archmoj commented Jan 18, 2023

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
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
Contributor

Generally it looks like it works?

@nicolaskruchten
Copy link
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)

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

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 🚀

@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