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

Add amd wrapper to fix esbuild #7122

Merged
merged 2 commits into from
Aug 21, 2024

Conversation

birkskyum
Copy link
Contributor

@birkskyum birkskyum commented Aug 20, 2024

In order to use requirejs, the bundles need amd module compat

Related to

All but the plotly-with-meta bundle got this added, because if it's also added to plotly-with-meta that bundle will break the build.

It fixes requirejs, but now breaks amdefine.. sigh

@archmoj
Copy link
Contributor

archmoj commented Aug 20, 2024

Thanks very much for the PR.
Fixing requirejs is what we needed.

@archmoj
Copy link
Contributor

archmoj commented Aug 21, 2024

I'm curious to drop umd from esbuild config and instead add another umd wrapper similar to the one added by webpack without arrow functions.
See https://raw.githubusercontent.com/plotly/plotly.js/master/dist/plotly-basic.js

@birkskyum
Copy link
Contributor Author

birkskyum commented Aug 21, 2024

That's worth exploring too! Will you try it?

The build-in ESM support in browsers has caught up quite well (except in web workers which is a bit behind), so there is a future there where all this module wrapper ceremony becomes unneeded. But while interesting, that can only be considered for a major release, and maplibre does use web workers, so I don't know if that makes a problem for plotly too.

@gvwilson gvwilson added community community contribution P1 needed for current cycle fix fixes something broken labels Aug 21, 2024
@archmoj
Copy link
Contributor

archmoj commented Aug 21, 2024

I made some progress based on your PR.
Merging.

@archmoj archmoj merged commit d1a806a into plotly:build-with-esbuild Aug 21, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community community contribution fix fixes something broken P1 needed for current cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants