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

deps: upgrade @rollup/plugin-commonjs to v22 to fix try/catch requires #340

Merged
merged 1 commit into from
Jun 6, 2022

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Jun 3, 2022

@ezolenko this is a bit important as 0.32.0 is currently broken for some users. Ironically, I suggested calling out dependency changes in #332 (comment) and this issue is in fact due to a breaking change in a dependency. In a dev dependency at that!

Summary

Upgrade @rollup/plugin-commonjs to v22.0.0 to fix a breaking change in v21.0.0 that broke the bundling of graphlib and how it imports lodash, breaking rpt2 as a whole.

Details

  • v21 broke graphlib's internal usage of lodash with its breaking change, and v22 has another breaking change to fix that

    • notably, this was breaking rpt2 imports for some users because rpt2 bundles in graphlib etc, meaning the nuances of the bundling got leaked to consumers, and, in fact, broke their builds
  • run a build as well since that's necessary for/vital to this change

Can see more details in my root cause analysis in #339 (comment) and the comments leading up to that. rollup/plugins#1005 (comment) is the central bug that applies to this case too.

Review Notes

Note how massive a change it is to the dist due to just changing the version of @rollup/plugin-commonjs. I can't even link to the line in the diff where graphlib's usage of lodash changed because GitHub won't load a diff that big. I can confirm though that it does change that same area of code that was broken, and that testing this branch/PR in a user's repo resolved the issue.

I was able to repro in a user's library but couldn't repro this in rpt2 itself (when it bundles itself as in) or in one of my libraries though, so it's possibly an environment dependent bug or a race condition.

This will need a release once merged.

- and build
- v21 broke `graphlib`'s internal usage of `lodash` with its breaking
  change, and v22 has another breaking change to fix that
  - notably, this was breaking rpt2 imports for some users because rpt2
    bundles in `graphlib` etc, meaning the nuances of the bundling got
    leaked to consumers, and, in fact, broke their builds
    - another upside to unbundling
@agilgur5 agilgur5 requested a review from ezolenko June 3, 2022 14:50
@agilgur5 agilgur5 added scope: dependencies Issues or PRs about updating a dependency kind: regression Specific type of bug -- past behavior that worked is now broken labels Jun 3, 2022
@agilgur5 agilgur5 changed the title [important] deps: upgrade @rollup/plugin-commonjs to v22 to fix try/catch requires [important! merge and release me first!] deps: upgrade @rollup/plugin-commonjs to v22 to fix try/catch requires Jun 5, 2022
@agilgur5 agilgur5 changed the title [important! merge and release me first!] deps: upgrade @rollup/plugin-commonjs to v22 to fix try/catch requires !! [!important! merge and release me first!] deps: upgrade @rollup/plugin-commonjs to v22 to fix try/catch requires Jun 5, 2022
@ezolenko
Copy link
Owner

ezolenko commented Jun 6, 2022

Looks good (once you ignore all the whitespace :D). Do you think this should be 32.1 or 33.0? @agilgur5

@ezolenko ezolenko merged commit d7cab2d into ezolenko:master Jun 6, 2022
@quisido
Copy link

quisido commented Jun 6, 2022

This will need a release once merged.

I'm not sure the release process for this repository. Should this commit include a patch version bump?

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jun 7, 2022

once you ignore all the whitespace :D

Ah that's a good tip, I did not realize how much was whitespace changes

Do you think this should be 32.1 or 33.0? @agilgur5

@ezolenko 0.32.1 in my opinion, since 0.32.0 is partly broken due to this -- like literally unusable in certain environments. So can just consider this a "follow-up" release in that sense, since 0.32.0 wasn't entirely usable anyway.
In my opinion, I believe this to be a bugfix rather than a breaking change. I would also recommend deprecating 0.32.0 on NPM (in a week or so for less confusion) with a message saying to upgrade to 0.32.1.
This change should semantically make the dependencies work as they did in 0.31, and I think that's the main breakage in 0.32.0.
(The rest of the main changes are in 0.32.0 are "bugfixes that change some incorrect semantics"; could consider all of them non-breaking as well IMO).

Also, just for reference, I try to point out things one may consider as breaking in PRs too.

I'm not sure the release process for this repository. Should this commit include a patch version bump?

@CharlesStover generally there's a separate commit that ezolenko makes that only bumps versions.
That's done in most repos I've worked with, especially since a release may batch a few PRs together.

@ezolenko
Copy link
Owner

ezolenko commented Jun 7, 2022

Yep, release process goes roughly like this:

  • package version is set at least a patch version higher than previous release (usually this is done right after previous release)
  • if the changes look breaking, a minor version is incremented
  • I do a build, review the diff and publish on npm
  • build is then committed to git (if it wasn't already) and git release archive is made from that revision (so what's in npm and what's in git release under the same tag should match exactly)
  • package version is then incremented

@ezolenko
Copy link
Owner

ezolenko commented Jun 7, 2022

That second point (about minor version bump) is a pure judgement call. Hopefully it would be more substantiated eventually if we get integrations tests and stuff. :)

@ezolenko
Copy link
Owner

ezolenko commented Jun 7, 2022

This is released in 0.32.1 btw

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jun 7, 2022

Yep, release process goes roughly like this:

It might be good to add your exact steps, specifically the commands you run, into a "Publishing" section (under "Development") in the new CONTRIBUTING.md.
So that way it's explicitly described and other people can replicate it (and in the future maybe, so that others can release too). Also from personal experience, I legit sometimes forget my own publishing process if I haven't released in a bit 😅

That second point (about minor version bump) is a pure judgement call. Hopefully it would be more substantiated eventually if we get integrations tests and stuff. :)

💯 integration tests will probably be the next big thing I tackle as they're really starting to be necessary to make sure that some of the deeper bugfixes I'm making don't break a bunch of things.

But even then, version bumps are always subjective to an extent.
Even when I've tried to reason about a change with ComVer (which has no bugfixes), there are still some cases where something could be considered breaking, but was intended as more of a feature.
Or, in a broader sense, what one identifies as "the API surface" is itself subjective. For instance, a hypothetical: are thrown errors an API surface? In which case, changing an error message to be "more helpful" (i.e. a "feature") could also be considered breaking. 🤯 🤯 (actual scenario from one of my libraries: agilgur5/react-signature-canvas#42 (comment))

@agilgur5 agilgur5 changed the title !! [!important! merge and release me first!] deps: upgrade @rollup/plugin-commonjs to v22 to fix try/catch requires deps: upgrade @rollup/plugin-commonjs to v22 to fix try/catch requires Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: regression Specific type of bug -- past behavior that worked is now broken scope: dependencies Issues or PRs about updating a dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReferenceError: window is not defined in 0.32.0
3 participants