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

Build/bundle/lint overhaul #99

Merged
merged 6 commits into from
Sep 2, 2022
Merged

Build/bundle/lint overhaul #99

merged 6 commits into from
Sep 2, 2022

Conversation

akx
Copy link
Contributor

@akx akx commented Jul 6, 2022

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
Copy link
Collaborator

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
Copy link
Contributor Author

akx commented Jul 14, 2022

@alexcjohnson
Copy link
Collaborator

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
Copy link
Contributor Author

akx commented Jul 14, 2022

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
Copy link
Contributor Author

akx commented Jul 26, 2022

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

@alexcjohnson
Copy link
Collaborator

@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
Copy link
Contributor Author

akx commented Aug 30, 2022

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

Copy link
Collaborator

@alexcjohnson alexcjohnson left a 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!

@alexcjohnson alexcjohnson merged commit 3319ccc into plotly:master Sep 2, 2022
akx added a commit to akx/dash-cytoscape that referenced this pull request Sep 6, 2022
akx added a commit to akx/dash-cytoscape that referenced this pull request Sep 8, 2022
@akx akx deleted the microbundle branch February 3, 2023 15:44
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.

None yet

2 participants