-
-
Notifications
You must be signed in to change notification settings - Fork 66
Build/bundle/lint overhaul #99
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
Conversation
Hi @akx - thanks for the PR! From Plotly's side, we're happy to have folks using and contributing to this project, but for the most part what we care about is its use in dash-cytoscape and the dash devtools callback graph. So the main thing we would want to see before approving is a companion PR or two pulling this work into one or both of those, so we can see what if any impact it has there. |
Fantastic, thank you @akx! I'll take a close look at those two PRs but at first glance they look good. The only piece of this PR that worries me a little is the switch to microbundle. It's not a bundler we're familiar with at Plotly - though I can say we've experimented with rollup at various times, and for larger projects it hasn't worked out, but this one may be small enough that it's OK). Dash and its components are fairly invested in webpack via custom plugins that coordinate between dash-renderer and the components, so we're unlikely to want to change bundlers there. In this lower-level repo we certainly can envision using a different bundler without any conflict with webpack in those higher-level projects, but because this adds some extra surface area for potential maintenance issues I'd like to understand a little better the motivation for moving away from webpack. For example regarding multiple output targets, in dash-renderer we have two distinct outputs - for the dev and production builds. Can that same strategy be used here to get the multiple flavors of bundle you mention here? No concerns about all your other changes - looks excellent and very nicely done! And I agree that moving Cytoscape to a peer dependency is a semver major change. |
microbundle is indeed a small wrapper around Rollup and friends, and it's excellent at small libraries. I would choose something else (maybe Vite's library mode) these days for anything larger.
Simplicity and speed. Microbundle (well, Rollup) is much faster at doing its thing with almost zero configuration - and it generates compliant UMD/CJS/MJS bundles automagically to boot. Besides, since microbundle uses the standard entrypoint configuration format, it would be easy enough to move to another bundler should one need to.
Sure, you could use
Thank you! |
So, @alexcjohnson, is there something I can or should do to get this moving forward? 😄 Thanks! |
@akx apologies, I've been traveling - I just want to try building it myself using these changes, but you've convinced me everything here is reasonable. I should be able to get to it in the next couple of days. |
@alexcjohnson Sorry to keep prodding you, but any news on this front? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💃 Alright, finally got a chance to try it out myself - works great. Thanks for your patience @akx, I'll merge and release this as v2.0.0 shortly!
This PR hopes to breathe a bit of fresh air into the repo. If there's no action on this for a while, I'm planning to fork the project :)
In particular, it:
The PR looks big, but really, it's 99% just package-lock.json fluff.
I've tried that things work in a downstream project (bundled with Webpack) with
Let me know if there's anything I can do to get this moving forward.