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: bump Rollup peerDep to 3.7.5+, remove tech debt #458

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Jul 14, 2023

Summary

Bumping the Rollup peerDep and removing legacy tech debt workarounds

Details

Rollup upgrade

Other tech debt removals

Figured I'd remove some other deprecated code / tech debt while I'm at it

@agilgur5 agilgur5 added the scope: dependencies Issues or PRs about updating a dependency label Jul 14, 2023
src/index.ts Outdated
if (err || !pluginOptions.check)
return buildDone();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the logic here is a bit confusing -- I had to re-read my own code a few times.
Basically, I made sure the diff was functionally equivalent to the previous one, minus the error rewriting.

this.error causes an exit and return will similarly end the function.

If there's an error, we just want to call cache.done() (as buildDone() does) and otherwise let the error proceed.
If we're not doing type-checking, we can return early as the code below is only relevant when type-checking

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jul 14, 2023

Left this as a Draft PR for now as it is breaking, and I think we should get in a patch release and stabilize MTS/CTS support beforehand.

#453 would be ideal to get in beforehand as well, although that is technically a breaking change as well

And potentially there is more tech debt we want to remove in this PR as well; thought I might be missing something?

- 3.7.5 includes rollup/rollup@ffab4cd
  - which fixes the duplicate error logging upstream and allows us to remove the `buildEnd` workaround
- 2.60.0 includes `this.load`, so can remove the `satisfies` check
- 2.14.0 includes `this.meta.watchMode`, so can remove the env check

- remove deprecated `rollupCommonJSResolveHack`
  - it hasn't done anything since 6fb0e75 in late 2020 (~2.5 years ago)
  - and has been formally deprecated since 74f6761 over a year ago
- remove `objectHashIgnoreUnknownHack` warning
  - hasn't been needed for async functions since 9afc8df in early 2020 (~3.5 years ago)
    - so I think that's a long enough window to now remove the warning
  - also add a link in the docs to `object-hash`
    - noticed there wasn't one, despite all the links I added to the docs!
@agilgur5 agilgur5 added the version: minor Increment the minor version when merged label Jul 14, 2023
@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jul 14, 2023

ooo tests caught that devDeps need updating too! awesome to see integration tests reaping rewards 🙂
updating Rollup to 3+ requires updating several plugins as well, so I'm gonna need to review some breaking changes for those

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jul 14, 2023

Proactively bumped the repro template to use Rollup v3 (and TS v5 and rpt2 v0.35)

@ezolenko
Copy link
Owner

Last time I tried to just update everything updatable, I got a lot of grief from tsjest

@agilgur5
Copy link
Collaborator Author

Yea I noticed from #447 😅
ESM adoption has (expectedly) caused a bit of churn in the ecosystem.

Though this PR specifically only needs to update all the Rollup deps, so maybe won't need updating the rest. But seems I might need to do so for #460 anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: dependencies Issues or PRs about updating a dependency version: minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants