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

Components registry safe reload #5780

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Jun 6, 2024

Summary

In a Zeitwerk world, we can't use reloadable constant in initializers.
This moves the constantization of the names of components into a new
ComponentRegistry class.

This PR is build on top of #5779 so that the theory can directly be tested. Really relevant is only the second commit here.

@mamhoff mamhoff force-pushed the components-registry-safe-reload branch 3 times, most recently from 93a8d30 to c1826d7 Compare June 7, 2024 08:28
Copy link

codecov bot commented Jun 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.73%. Comparing base (6684502) to head (0408942).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5780   +/-   ##
=======================================
  Coverage   88.72%   88.73%           
=======================================
  Files         712      713    +1     
  Lines       16914    16924   +10     
=======================================
+ Hits        15007    15017   +10     
  Misses       1907     1907           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mamhoff mamhoff marked this pull request as ready for review June 7, 2024 08:48
@mamhoff mamhoff requested a review from a team as a code owner June 7, 2024 08:48
@mamhoff mamhoff force-pushed the components-registry-safe-reload branch 2 times, most recently from 83ef1ac to 4005895 Compare June 11, 2024 16:02
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Loving where this is going, but we should give caching a try IMO.

The Rails guide even says:

Bottom line: do not cache reloadable classes or modules.

Is there a way to use Zeitwerk for the cache? 🤔

admin/lib/solidus_admin/configuration.rb Show resolved Hide resolved
admin/lib/solidus_admin/component_registry.rb Show resolved Hide resolved
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Yeah. You are right. I think we can tackle the caching part in another PR. We haven't have caching before. So, this is already an improvisation. Thanks

This adds a new `ComponentRegistry` class that allows:
- setting component classes as strings (to allow for easy reloading)
- getting nice errors when requesting components that don't exist
- getting nice errors when setting a component to a non-existing class

It also moves some of the logic from `SolidusAdmin::Configuration` into
a separate class.

I've decided not to add memoization here as I'm not sure this would make
a giant difference performance-wise, and also the Rails Guide on
Reloading constants very clearly says:

```
Bottom line: do not cache reloadable classes or modules.
```
@mamhoff mamhoff force-pushed the components-registry-safe-reload branch from 4005895 to 0408942 Compare June 12, 2024 07:48
@mamhoff
Copy link
Contributor Author

mamhoff commented Jun 12, 2024

Rebased on main to account for the flaky spec.

@mamhoff mamhoff closed this Jun 12, 2024
@mamhoff mamhoff reopened this Jun 12, 2024
@tvdeyen tvdeyen merged commit 44603e1 into solidusio:main Jun 12, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants