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

feat: use UMD instead of IIFE for browser target #336

Merged
merged 1 commit into from
Apr 11, 2021
Merged

feat: use UMD instead of IIFE for browser target #336

merged 1 commit into from
Apr 11, 2021

Conversation

himynameisdave
Copy link
Contributor

Explanation About What Code Achieves:

This is an attempt to fix #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:

  • Users who install this dependency and consume it the "modern way", via installing and importing it (as described in the README):
    import { Chart } from 'frappe-charts';
  • By importing from frappe-charts (not a specific file like .esm.js), we are allowing our bundler to resolve what frappe-charts means.
  • Generally bundlers mostly align with Node's module resolution algorithm. Some have more options. Here's webpack's.
  • This means that Node will reach into frappe-charts, inspect its package.json and hopefully resolve to some entry file specified there. Luckily frappe-charts specifies this.
  • The error happens because some users, who are bundling JS for the browser, correctly set browser as the target in their bundler. This resolves to the .iife.js file currently.
  • IIFE files do not export anything / cannot be consumed in those environments. This is why we are seeing this error.

Users can probably get around this issue in their bundler with some Regex gymastics / explicitly telling frappe-charts to resolve to not the browser 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:
  • N/A
Steps To Test:
  • One good test would be to run npm run build, grab the .umd. output file, stick it in an HTML file and open in a browser and ensure that window.frappe.Chart is still available and works as expected.
  • I will put together a repo where we can test both import setups and ensure this is fixed.
TODOs:
  • We may want to update our build dependencies, especially rollup and associated plugins. They have made massive improvements since the version we are using now.
  • I didn't upload any of the built 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.

@scmmishra
Copy link
Contributor

scmmishra commented Apr 2, 2021

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

@scmmishra scmmishra self-requested a review April 2, 2021 05:38
@scmmishra scmmishra self-assigned this Apr 2, 2021
@scmmishra scmmishra changed the title Use UMD instead of IIFE for browser target feat: use UMD instead of IIFE for browser target Apr 11, 2021
@scmmishra scmmishra merged commit 8e1bd68 into frappe:master Apr 11, 2021
@scmmishra
Copy link
Contributor

This looks great! I've merged this and made a release. Thanks a lot for your contribution!

@GrosSacASac
Copy link
Contributor

Nice

@himynameisdave himynameisdave deleted the umd-browser-build branch April 11, 2021 21:56
@barredterra
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'Chart is not a constructor' when importing as ES6 module
4 participants