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

Use adapter method for Shakapacker/Webpacker constant #1547

Closed
wants to merge 12 commits into from

Conversation

Judahmeek
Copy link
Contributor

@Judahmeek Judahmeek commented Jun 14, 2023

Summary

This change decouples ReactOnRails from the Webpacker constant, enabling ReactOnRails to support both Webpacker v5 & Shakapacker v7 without any breaking changes.

Pull Request checklist

Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by ~.

  • Add/update test to cover these changes
  • Update documentation
  • Update CHANGELOG file
    Add the CHANGELOG entry at the top of the file.

Other Information

Remove this paragraph and mention any other important and relevant information such as benchmarks.


This change is Reviewable

@ahangarha
Copy link
Contributor

This is an interesting implementation. It is a learning opportunity for me.

But I wonder if we need to add such complexity to this project while migrating to Shakapacer 7 is easy and straightforward. Adding an adapter would be good if we have a dependency with a complex upgrade, but in this case, encouraging migration is a better approach.

@Judahmeek
Copy link
Contributor Author

But I wonder if we need to add such complexity to this project while migrating to Shakapacer 7 is easy and straightforward. Adding an adapter would be good if we have a dependency with a complex upgrade, but in this case, encouraging migration is a better approach.

You're probably right, although I don't think these changes actually add that much more complexity to our code base.

What this change does do is enable any ReactOnRails users who are still using Webpacker 5 (which I believe we're still supporting? We never formally dropped support for it) to continue using Webpacker & enjoy the latest bug fixes, such as #1540 or #1544.

If we don't support Webpacker 5, then I would agree that the transition from Shakapacker v6 to Shakapacker v7 should be smooth enough that these changes are unnecessary.

@Judahmeek Judahmeek requested a review from justin808 June 15, 2023 15:23
@justin808
Copy link
Member

Seems like this can work, but is this worth the extra complexity? Can we really have tests for both shakapacker and webpacker dependencies?

Could we just change to Shakapacker for the next version? Why or why not?

@Judahmeek
Copy link
Contributor Author

@justin808

Seems like this can work, but is this worth the extra complexity?

The only benefit over a simple replace & major version bump is that this would enable users still using Webpacker to enjoy any non-Shakapacker specific fixes or functionality that we add in the future.

Since we don't intend to support any other webpack wrapper, this complexity is likely overkill.

Can we really have tests for both shakapacker and webpacker dependencies?

We could, although that would probably require overhauling the test suite to be similar to react-rails' test suite & possibly even the generators for ReactOnRails.

If we were going to merge this PR, I'd probably go with not changing the test suite & handling any webpacker regressions as clients report them.

Could we just change to Shakapacker v7 for the next version?

It'll require a major version bump for RoR since we're changing the "optional" dependency, but yes.

Then we'll just tell users to upgrade Shakapacker & ReactOnRails together.

@justin808
Copy link
Member

@Judahmeek @ahangarha where do we stand on this one?

@ahangarha
Copy link
Contributor

I am not in a right position to give opinion on this. I prefer to learn from the decisions.

But I think we don't need to do this unless there is a necessity.

@Judahmeek
Copy link
Contributor Author

Closing in preference for #1546

@Judahmeek Judahmeek closed this Jun 29, 2023
@Judahmeek Judahmeek deleted the judahmeek/packer-adapter branch June 29, 2023 00:27
@ahangarha
Copy link
Contributor

I think #1548 satisfies our needs for now.

@Judahmeek Judahmeek restored the judahmeek/packer-adapter branch June 11, 2024 05:57
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.

3 participants