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

Introducing zeitwerk #2363

Merged
merged 25 commits into from
Apr 11, 2024
Merged

Conversation

ericproulx
Copy link
Contributor

@ericproulx ericproulx commented Oct 28, 2023

In the past, I tried to make some changes regarding autoload #2200, #2207, #2209. Unfortunately, #2207 and #2209 had to be reverted.

After reading this article, I thought it would be great to use zeitwerk instead of autoload.

Benefits

  1. manages the autoload
  2. eager_load proof
  3. no explicit requires except dependencies

Changes

  • Applied the file convention structure
  • eager_load is done when loading grape instead of at compile!
  • eager_load_spec has been removed
  • Removed the validator registration process (were loaded dynamically through inherited)
  • Removed the cache for coercers (@collection_coercers)

Change some classes base on file structure
Remove all require 'grape/**/*'
Remove active_support/autoload
Replace eager_load file by zeitwerk eager_load
Move and rename Grape.lowercase_headers? to Grape::Http::Headers.lowercase?
Add headers_spec conditions before running spec
Move lazy_value_*** to their own classes
Replace parse_media_type_patch by Grape::Util::Accept (stop overriding Rack:Accept::Header)
Remove validator registration and leverage Zeitwerk
Move instance_behaviour_spec.rb specs in custom_validations_spec.rb
Remove i18n dependency
Remove Coercer collection
@dblock
Copy link
Member

dblock commented Nov 1, 2023

What's the downside?

@ericproulx
Copy link
Contributor Author

What's the downside?

Follow the file structure convention if we can label that has a "downside".

@ericproulx ericproulx marked this pull request as ready for review November 13, 2023 21:50
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Looks good. Some comments/asks. Let's increment the version to 2.1.0 as part of this PR since this is not a small change?

lib/grape.rb Show resolved Hide resolved
UPGRADING.md Outdated Show resolved Hide resolved
@dblock
Copy link
Member

dblock commented Mar 24, 2024

Rebase? I'll take a look.

@ericproulx ericproulx requested a review from dblock March 24, 2024 18:31
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

I'd like to merge this.

  1. Update the language around auto/lazy loading/reloading in README.
  2. What externally visible contracts are we breaking? Should we be incrementing the version to 3.0?

@ericproulx
Copy link
Contributor Author

I'd like to merge this.

1. Update the language around auto/lazy loading/reloading in README.

2. What externally visible contracts are we breaking? Should we be incrementing the version to 3.0?

There's a breaking change with custom validator. Now, they must follow the convention Grape::Validations::Validators::[CustomName] The registration part was done through inheritance. I'll put it back.

@dblock
Copy link
Member

dblock commented Apr 1, 2024

There's a breaking change with custom validator. Now, they must follow the convention Grape::Validations::Validators::[CustomName] The registration part was done through inheritance. I'll put it back.

Sounds good, I'll wait to merge.

@ericproulx
Copy link
Contributor Author

ericproulx commented Apr 1, 2024

@dblock I've found something else related to gem grape-swagger. Versions below 2.0 are grape ~> 1.3 and ruby >= 2.7. Above 2.0, its ruby >= 3.0 and 'grape', '>= 1.7', '< 3.0'.

Next Grape release is 2.1.0 and Grape is still ruby >= 2.7.0. Should they follow each other related to ruby version ?

@dblock
Copy link
Member

dblock commented Apr 1, 2024

Next Grape release is 2.1.0 and Grape is still ruby >= 2.7.0. Should they follow each other related to ruby version ?

I don't think they have to. Generally we remove rubies going EOL or if there's "good reason". If you can spell it out in a PR we can do whatever :)

@dblock
Copy link
Member

dblock commented Apr 1, 2024

The code looks good here. I think we still need to say something in UGPRADING/README about auto-loading? WDYT? It feels like a scary change to go "quietly" :)

@dblock
Copy link
Member

dblock commented Apr 10, 2024

@ericproulx Want to add some words to those files and let's merge?

@ericproulx
Copy link
Contributor Author

ericproulx commented Apr 10, 2024

@ericproulx Want to add some words to those files and let's merge?

Done. I mentionned something about Monkey Patch that might result in a Zeitwerk Error.

@dblock dblock merged commit 2c79b2f into ruby-grape:master Apr 11, 2024
38 checks passed
@dblock
Copy link
Member

dblock commented Apr 11, 2024

Merged, nice work.

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.

2 participants