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: use Zeitwerk to load gem structure #46

Closed
wants to merge 1 commit into from

Conversation

seuros
Copy link
Contributor

@seuros seuros commented Nov 27, 2022

dry-rb use zeitwerk as loader starting from version 1.0.0

I leveraged it usage to make it work in this gem.

@waiting-for-dev
Copy link
Owner

Thanks, I'll look into it as soon as possible.

@thomasdarde thomasdarde mentioned this pull request Jan 19, 2023
7 tasks
@thomasdarde
Copy link

This branch works nice for us in case it helps merging it

@dmitry
Copy link

dmitry commented Jan 20, 2023

@waiting-for-dev please let us know if it's still required some work for this PR.

@waiting-for-dev
Copy link
Owner

Many thanks for your collaboration. TBH, I don't feel comfortable adding Zeitwerk as a dependency. I know it's already transitive through dry-*, but I don't think adding the complexity here pays off. WDYT?

(I'm ok relaxing dry-* requirements, though, I already commented in the referenced PR)

@seuros
Copy link
Contributor Author

seuros commented Jan 22, 2023

@waiting-for-dev , I did notice a slight performance speed in testing by bumping to dry 1.0. (classes are loaded only when needed)
This is the reason I think we should start encouraging other devs to keep their dependencies up to date.

@waiting-for-dev
Copy link
Owner

To be clear, I'm ok bumping to dry 1.0 (not only ok, we need to do it), but I'm not sure about using Zeitwerk here.

@thomasdarde
Copy link

Hi @waiting-for-dev , do you have any news on this 1.0 release schedule ?

@waiting-for-dev
Copy link
Owner

Hi @waiting-for-dev , do you have any news on this 1.0 release schedule ?

I just released warden-jwt_auth v0.8.0 🥳

I'll close this PR for now, but I might reconsider it in the future. Thanks again for your contribution.

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.

4 participants