Moving API attribute helpers to API config #4039
Conversation
This commit moves the helper attributes location from the helper itself to the api configuration class. This fixes an error with modern Rails autoload in development. [4027] Fixing hound issues [4027] Removing a leftover comment
There was a problem hiding this comment.
Thanks @snada, looks promising!
I left a question, and I'd love to see a spec that actually describes the deprecated behavior as well. For example testing that accessing/setting the attribute in the old way still works raising a deprecation warning. Here's an example at an old ref where we do something similar. It is no more in the main branch because we recently released Solidus 3.0 and removed deprecated code (and associated specs). What do you think?
| define_singleton_method attribute do | ||
| Spree::Deprecation.warn("Please use Spree::Api::Config::#{attribute} instead.") | ||
| Spree::Api::Config.send(attribute) | ||
| end |
There was a problem hiding this comment.
I'm not sure to get why we need both define_method and define_singleton_method here. Can you please help me with some details?
There was a problem hiding this comment.
Sure. Methods defined with define_method are the ones used in views. Remove that definition and you'll see every test rendering something using these methods fail.
The singleton methods are the ones called when you reference to the helper directly, like so:
2.7.0 :015 > Spree::Api::ApiHelpers.user_attributes
DEPRECATION WARNING: Please use Spree::Api::Config::user_attributes instead. (called from irb_binding at (irb):15)
=> [:id, :email, :created_at, :updated_at, :likes_count]
You can clearly see the deprecation warning. Without that definition every piece of code currently using will break (like some of our tests, that I changed in this PR). With this code you will get the deprecation warning.
Plus, the very definition of mattr_reader (previously used to generate these methods) explains how it creates both a class and an instance reader for the provided symbol.
I just pushed a commit with tests regarding the deprecation warning.
kennyadsl
added
Important Change
changelog:solidus_api
kennyadsl
changed the title
Please use Spree::Api::Config now. Ref solidusio#4039
For that, please use Spree::Api::Config preferences now. The helper attributes can be only used when the module is included (eg. in controllers/views context). Ref solidusio#4039
For that, please use Spree::Api::Config preferences now. The helper attributes can be only used when the module is included (eg. in controllers/views context). Ref solidusio#4039
For that, please use Spree::Api::Config preferences now. The helper attributes can be only used when the module is included (eg. in controllers/views context). Ref solidusio#4039
For that, please use Spree::Api::Config preferences now. The helper attributes can be only used when the module is included (eg. in controllers/views context). Ref solidusio#4039
For that, please use Spree::Api::Config preferences now. The helper attributes can be only used when the module is included (eg. in controllers/views context). Ref #4039
Issue: #4027
Edgeguides Pr: solidusio/edgeguides#37
Description
As described in this issue there is a problem with the way data is autoloaded when pushing new attributes in an initalizer.
A proposed change was to move the attributes location from the helper module to the api config class.
This PR follows this approach.
Albeit this PR does not change existing behavior, it introduces a deprecation when directly accessing attributes from the static helper methods.
To take advantage of the autoload fix, attributes in initializers should be set this way:
Edgeguides has been updated accordingly.
Checklist: