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

feat(deps): updates and adds support for rollup v3 #25

Merged
merged 2 commits into from
Sep 4, 2023

Conversation

geotrev
Copy link

@geotrev geotrev commented Aug 23, 2023

I think this should be a somewhat simple PR but let me know if there are additional things I should test for... the existing plugins seem to hold up.

Addresses #24

Copy link
Owner

@brodybits brodybits left a comment

Choose a reason for hiding this comment

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

My apologies for the delay, I think we need to add CI testing with Rollup v2 as well.

Maybe the best thing would be to check the output sizes less precisely with Rollup v2. To do this in CI, I would recommend an approach like this:

  • add CI task that adds specific npm or Yarn installation of Rollup v2, test with an environmental variable defined if using Rollup v2
  • then test some sizes less precisely or consider other adjustments in case of Rollup v2

As an alternative: we could also drop support for Rollup v2 in a new 0.x release of this Rollup plugin. I think this could be an acceptable alternative if we document it somewhere as this fork does not seem to have too many users.

Thanks for your contribution, definitely much appreciated!

@geotrev
Copy link
Author

geotrev commented Aug 30, 2023

@brodybits thanks for taking a look!

Do you anticipate new/changing features that would make backwards compatibility on Rollup v2 beneficial? If not, bumping to a new minor and dropping v2 support works for me. In that case, I can update the README.

Give it some thought, I'm cool making changes in either direction. 👍🏻

@brodybits
Copy link
Owner

My first choice would be to support both Rollup v2 & v3 with testing, raised #26 to track this.

I can merge this PR for a new minor release if we remove the Rollup v2 option, and resolve #26 whenever someone has time to work on this.

Thanks!

package.json Outdated Show resolved Hide resolved
@geotrev
Copy link
Author

geotrev commented Sep 4, 2023

Sounds good, thanks.

Copy link
Owner

@brodybits brodybits left a comment

Choose a reason for hiding this comment

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

Approved & merging with removal of Rollup v2 support, as already discussed, thanks!

@brodybits brodybits merged commit a6e0a09 into brodybits:main Sep 4, 2023
21 checks passed
@brodybits brodybits mentioned this pull request Sep 5, 2023
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