plotly / react-cytoscapejs

React component for Cytoscape.js network visualisations
MIT License
470 stars 69 forks source link

Build/bundle/lint overhaul #99

Closed akx closed 2 years ago

akx commented 2 years ago

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

npm run clean && npm run build && npm pack
yarn add ../react-cytoscapejs/*.tgz

Let me know if there's anything I can do to get this moving forward.


alexcjohnson commented 2 years ago

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.

akx commented 2 years ago

@alexcjohnson

Sure thing. Please see:

alexcjohnson commented 2 years ago

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.

akx commented 2 years ago

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

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.

I'd like to understand a little better the motivation for moving away from webpack.

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.

For example regarding multiple output targets, in dash-renderer we have [...] dev and production builds.

Sure, you could use --define to e.g. set process.env.NODE_ENV and --compress=false to avoid minification, for a more developmentesque build. That said, microbundle does generate source maps as it goes, so for a simple library (which is what it's for), a source map should be enough to help pin down bugs in production environments.

No concerns about all your other changes - looks excellent and very nicely done!

Thank you!

akx commented 2 years ago

So, @alexcjohnson, is there something I can or should do to get this moving forward? 😄 Thanks!

alexcjohnson commented 2 years ago

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

akx commented 2 years ago

@alexcjohnson Sorry to keep prodding you, but any news on this front? :)