Skip to content

clean: remove backward-compat checks for old Rollup versions#374

Merged
ezolenko merged 1 commit intoezolenko:masterfrom
agilgur5:clean-remove-old-rollup-checks
Jul 6, 2022
Merged

clean: remove backward-compat checks for old Rollup versions#374
ezolenko merged 1 commit intoezolenko:masterfrom
agilgur5:clean-remove-old-rollup-checks

Conversation

@agilgur5
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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