Skip to content

Conversation

@agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Jul 2, 2022

Summary

Clean up some old backward-compat checks that are no longer necessary for the minimum version of Rollup that rpt2 requires

Details

Future Work

  • More follow-ups to refactor: prefer native methods to lodash where possible #328 to remove more lodash usage
    • The rest of the _.isFunction usage can be replaced with a check for the opposite condition, i.e. typeof message === "string" ? message : message()
    • _.merge, _.isEqual, and _.compact would be the only ones leftover after

- we require Rollup `>=1.26.3` in the peerDeps and the README and have for years now
  - (and Rollup is currently at `2.75.7`, so this is a pretty low target)
  - as such, we can remove various checks that are legacy backward-compat remnants for old Rollup versions:
    - `this.meta` was added to `options` in `1.1.0`: https://github.com/rollup/rollup/blob/master/CHANGELOG.md#110
    - `this.addWatchFile` was added at least in `1.0.0`: https://github.com/rollup/rollup/blob/master/CHANGELOG.md#100
    - `this.error` and `this.warn` were added to `transform` in `0.41.0`: https://github.com/rollup/rollup/blob/master/CHANGELOG.md#0410

- this simplifies some of the code a decent bit, `RollupContext` in particular
  - see also the usage of `buildEnd` in my other PR

- modify tests to account for these changes; basically just simplify them
  - and remove the check against private internals of the class, as they're private
    - guess I forgot to remove this when I was refactoring the test code?
@agilgur5 agilgur5 added kind: internal Changes only affect the internals, and _not_ the public API or external-facing docs scope: tests Tests could be improved. Or changes that only affect tests labels Jul 2, 2022
@ezolenko
Copy link
Owner

ezolenko commented Jul 6, 2022

btw, there is no reason not to bump minimum rollup version if we need to use anything new, I expect most people either update whole build toolchain, or freeze the lot. Well, all sane people would anyway... :)

@ezolenko ezolenko merged commit d078f3f into ezolenko:master Jul 6, 2022
@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jul 6, 2022

btw, there is no reason not to bump minimum rollup version if we need to use anything new

Yea I figure, given how old the current min is, but that also would technically be a breaking change, so also wouldn't want to do so without good reason. So far, I haven't encountered anything that needed new APIs though (which is surprising, because as a Rollup user, it seemed like I had to update quite a lot for Rollup 2, but now, as a plugin developer, I'm not seeing the same need to upgrade 🤷 )

@agilgur5 agilgur5 deleted the clean-remove-old-rollup-checks branch July 2, 2023 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind: internal Changes only affect the internals, and _not_ the public API or external-facing docs scope: tests Tests could be improved. Or changes that only affect tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants