Closed himynameisdave closed 3 years ago
Thanks a lot @himynameisdave I really appreciate your help on this. I'll test this PR over the weekend and release it if all works fine
This looks great! I've merged this and made a release. Thanks a lot for your contribution!
Nice
@himynameisdave this is one of the best PR descriptions i've ever seen. Thanks for making the effort and let's hope others will take it as an example.
Explanation About What Code Achieves:
This is an attempt to fix https://github.com/frappe/charts/issues/199 (as well as downstream issues in my
svelte-frappe-charts
wrapper library).I tried to explain why this is happening here. Allow me to summarize:
frappe-charts
(not a specific file like.esm.js
), we are allowing our bundler to resolve whatfrappe-charts
means.frappe-charts
, inspect itspackage.json
and hopefully resolve to some entry file specified there. Luckilyfrappe-charts
specifies this.browser
as the target in their bundler. This resolves to the.iife.js
file currently.Users can probably get around this issue in their bundler with some Regex gymastics / explicitly telling
frappe-charts
to resolve to not thebrowser
version. Or they can do what the README says and manually specify the path to the.esm.js
.The user shouldn't have to do either of these. Also, how are libraries which consume
frappe-charts
meant to import it? (we believe they should be agnostic and let the bundler decide).This PR tells Rollup to produce a UMD-style bundle, and sets that as the
browser
entry point.📄 You can read more about IIFE vs UMD here.
Screenshots/GIFs:
Steps To Test:
npm run build
, grab the.umd.
output file, stick it in an HTML file and open in a browser and ensure thatwindow.frappe.Chart
is still available and works as expected.TODOs:
rollup
and associated plugins. They have made massive improvements since the version we are using now.dist
files, since I assumed that they get built before release. They are in git though, so let me know if you want me to push them.