Clean up `require ‘active_support/deprecation’` and remove circular require #12861

Merged
merged 1 commit into from Nov 14, 2013

Conversation

Projects
None yet
4 participants
@route
Contributor

route commented Nov 12, 2013

When I do require 'active_support/core_ext',
I'm getting NameError: uninitialized constant ActiveSupport::Deprecation::MethodWrapper

With this commit 77b587f require 'active_support/deprecation' was removed from date_time/calculations.rb, this file was loaded before require 'active_support/core_ext/module/deprecation' and was the reason why it had worked. Anyway I consider require 'active_support/deprecation/method_wrappers' useless because it just defines a module that has to be mixed into Deprecation.

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Nov 12, 2013

Member

There was also #11314 to solve the same problem when requiring active_support/all.

Member

senny commented Nov 12, 2013

There was also #11314 to solve the same problem when requiring active_support/all.

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Nov 12, 2013

Member

@route will your fix also solve this issue and we could remove the require from all.rb?

Member

senny commented Nov 12, 2013

@route will your fix also solve this issue and we could remove the require from all.rb?

@route

This comment has been minimized.

Show comment Hide comment
@route

route Nov 12, 2013

Contributor

Checking that...

Contributor

route commented Nov 12, 2013

Checking that...

@route

This comment has been minimized.

Show comment Hide comment
@route

route Nov 12, 2013

Contributor

Ok it's not that simple... still working

Contributor

route commented Nov 12, 2013

Ok it's not that simple... still working

@route

This comment has been minimized.

Show comment Hide comment
@route

route Nov 12, 2013

Contributor

I have looked through AS and found a few places where Deprecation was used but never required. Also I've updated description of this PR so @senny please reread it and I'd like to fill in @fxn. He knows about circular require. I want you to show what I came up with. It seems like 'active_support/deprecation' needs 'active_support/core_ext/module/deprecation' and vise versa. We have to add missed require 'active_support/deprecation' to 'module/deprecation' but it follows us to circular dependency, because 'active_support/deprecation' also requires 'module/deprecation'. So I have to options here: 1) Take out the code into another file(as I did) 2) Add such constructions as require ... unless respond_to?(:deprecate) and const_defined? 3) Put require in Module into method definition. Thoughts?

Contributor

route commented Nov 12, 2013

I have looked through AS and found a few places where Deprecation was used but never required. Also I've updated description of this PR so @senny please reread it and I'd like to fill in @fxn. He knows about circular require. I want you to show what I came up with. It seems like 'active_support/deprecation' needs 'active_support/core_ext/module/deprecation' and vise versa. We have to add missed require 'active_support/deprecation' to 'module/deprecation' but it follows us to circular dependency, because 'active_support/deprecation' also requires 'module/deprecation'. So I have to options here: 1) Take out the code into another file(as I did) 2) Add such constructions as require ... unless respond_to?(:deprecate) and const_defined? 3) Put require in Module into method definition. Thoughts?

@route

This comment has been minimized.

Show comment Hide comment
@route

route Nov 13, 2013

Contributor

Hey @senny WDYT?

Contributor

route commented Nov 13, 2013

Hey @senny WDYT?

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Nov 13, 2013

Member

I'll leave this one up to @fxn

Member

senny commented Nov 13, 2013

I'll leave this one up to @fxn

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Nov 13, 2013

Owner

At first sight I have two remarks:

One, a guiding principle in modern AS is that every core extension should be self-contained, so that you can cherry-pick whatever you need and thus keep a minimal footprint. In particular, module/deprecation.rb does not follow this guideline, it uses AS::Deprecation so it should import it.

On the other hand, in my view this file has no business loading method_wrappers.rb. AS::Deprecation is responsible for building its interface.

I need to think a bit about this one.

Owner

fxn commented Nov 13, 2013

At first sight I have two remarks:

One, a guiding principle in modern AS is that every core extension should be self-contained, so that you can cherry-pick whatever you need and thus keep a minimal footprint. In particular, module/deprecation.rb does not follow this guideline, it uses AS::Deprecation so it should import it.

On the other hand, in my view this file has no business loading method_wrappers.rb. AS::Deprecation is responsible for building its interface.

I need to think a bit about this one.

@route

This comment has been minimized.

Show comment Hide comment
@route

route Nov 13, 2013

Contributor

Totally agree, that's why requiring method_wrappers.rb useless since it's just a module that will be used in active_support/deprecation.rb, it turns out we need active_support/deprecation.rb there rather than method_wrappers.rb, but active_support/deprecation.rb in its turn needs module/deprecation that drives us to circular dependency. I've pulled it off to another module as the solution. I'm waiting for your thoughts.

Contributor

route commented Nov 13, 2013

Totally agree, that's why requiring method_wrappers.rb useless since it's just a module that will be used in active_support/deprecation.rb, it turns out we need active_support/deprecation.rb there rather than method_wrappers.rb, but active_support/deprecation.rb in its turn needs module/deprecation that drives us to circular dependency. I've pulled it off to another module as the solution. I'm waiting for your thoughts.

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Nov 13, 2013

Owner

I am not sold on the proposed solution. We open module/deprecation.rb and see a very surprising content: just two requires, this file has the dependencies of another file. Then, Module#deprecate is defined in a file that is special because users of AS are not supposed to load it, it is there only as a hack. Also, the API would report this method is defined in the private file, which is something we do not want (maybe there could be an extra workaround for that).

Let me think a bit about it, also let me ping @jeremy in case he'd like to propose something.

Owner

fxn commented Nov 13, 2013

I am not sold on the proposed solution. We open module/deprecation.rb and see a very surprising content: just two requires, this file has the dependencies of another file. Then, Module#deprecate is defined in a file that is special because users of AS are not supposed to load it, it is there only as a hack. Also, the API would report this method is defined in the private file, which is something we do not want (maybe there could be an extra workaround for that).

Let me think a bit about it, also let me ping @jeremy in case he'd like to propose something.

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Nov 13, 2013

Owner

We are going to solve this like it is done with json.

Users have to load active_support.rb in order to use Active Support, therefore we have an autoload set up for AS::Deprecation.

What we are going to do not to have any require in module/deprecation.rb. If the file is cherry-picked, loading the file will finish just fine, and when the delegate method is called the autoload will be triggered.

If we go the other way around it works just fine.

Owner

fxn commented Nov 13, 2013

We are going to solve this like it is done with json.

Users have to load active_support.rb in order to use Active Support, therefore we have an autoload set up for AS::Deprecation.

What we are going to do not to have any require in module/deprecation.rb. If the file is cherry-picked, loading the file will finish just fine, and when the delegate method is called the autoload will be triggered.

If we go the other way around it works just fine.

@route

This comment has been minimized.

Show comment Hide comment
@route

route Nov 13, 2013

Contributor

So this way users have to require module/deprecation like this if they want to use it:

require 'active_support'
require 'active_support/core_ext/module/deprecation'

Did I get you right?

Contributor

route commented Nov 13, 2013

So this way users have to require module/deprecation like this if they want to use it:

require 'active_support'
require 'active_support/core_ext/module/deprecation'

Did I get you right?

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Nov 13, 2013

Owner

Exactly.

Owner

fxn commented Nov 13, 2013

Exactly.

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Nov 13, 2013

Owner

In general, if you use any of the Rails components you need to load its root file, and this recent commit makes it clear in the AS guide.

Owner

fxn commented Nov 13, 2013

In general, if you use any of the Rails components you need to load its root file, and this recent commit makes it clear in the AS guide.

@chancancode

View changes

activesupport/lib/active_support/deprecation.rb
@@ -17,7 +17,7 @@ class Deprecation
require 'active_support/deprecation/reporting'
require 'active_support/deprecation/method_wrappers'
require 'active_support/deprecation/proxy_wrappers'
- require 'active_support/core_ext/module/deprecation'
+ require 'active_support/core_ext/module/deprecate'

This comment has been minimized.

Show comment Hide comment
@chancancode

chancancode Nov 14, 2013

Member

Is this necessary? The require path is considered a public API (people might be requiring this path today), so if you make this change you'd have to make sure the old one still works with a deprecation warning (see the to_json -> json change). In this cause I doubt that it's worth it since the original name still fits the content quite well.

@chancancode

chancancode Nov 14, 2013

Member

Is this necessary? The require path is considered a public API (people might be requiring this path today), so if you make this change you'd have to make sure the old one still works with a deprecation warning (see the to_json -> json change). In this cause I doubt that it's worth it since the original name still fits the content quite well.

@chancancode

View changes

activesupport/lib/active_support/core_ext/object/to_json.rb
@@ -1,5 +1,6 @@
+require 'active_support/deprecation'

This comment has been minimized.

Show comment Hide comment
@chancancode

chancancode Nov 14, 2013

Member

This shouldn't be necessary since we can assume autoload

@chancancode

chancancode Nov 14, 2013

Member

This shouldn't be necessary since we can assume autoload

@chancancode

View changes

activesupport/lib/active_support/json/encoding.rb
@@ -1,5 +1,6 @@
#encoding: us-ascii
+require 'active_support/deprecation'

This comment has been minimized.

Show comment Hide comment
@chancancode

chancancode Nov 14, 2013

Member

ditto

@chancancode

View changes

activesupport/lib/active_support/core_ext/module/deprecation.rb
- ActiveSupport::Deprecation.deprecate_methods(self, *method_names)
- end
-end
+require 'active_support/deprecation'

This comment has been minimized.

Show comment Hide comment
@chancancode

chancancode Nov 14, 2013

Member

This should be removed

@chancancode

chancancode Nov 14, 2013

Member

This should be removed

@route

This comment has been minimized.

Show comment Hide comment
@route

route Nov 14, 2013

Contributor

@fxn I've updated PR could you please check it out?

Contributor

route commented Nov 14, 2013

@fxn I've updated PR could you please check it out?

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Nov 14, 2013

Owner

I believe it would be better not to add the require to callbacks. In general, the goal of autoloading is to defer loading the code until/if it is needed.

Owner

fxn commented Nov 14, 2013

I believe it would be better not to add the require to callbacks. In general, the goal of autoloading is to defer loading the code until/if it is needed.

@route

This comment has been minimized.

Show comment Hide comment
@route

route Nov 14, 2013

Contributor

Done

Contributor

route commented Nov 14, 2013

Done

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Nov 14, 2013

Owner

Awesome, thanks a lot for your work on this PR ❤️.

Owner

fxn commented Nov 14, 2013

Awesome, thanks a lot for your work on this PR ❤️.

fxn added a commit that referenced this pull request Nov 14, 2013

Merge pull request #12861 from route/missed_require_for_module
Clean up `require ‘active_support/deprecation’` and remove circular require

@fxn fxn merged commit 436a9c1 into rails:master Nov 14, 2013

@route

This comment has been minimized.

Show comment Hide comment
@route

route Nov 14, 2013

Contributor

@fxn to make it clear for me, if I've got to use some component inside active_support I should be ready to write require 'active_support' before requiring needed component? Is the any exception or we have to do it anyway?

Contributor

route commented Nov 14, 2013

@fxn to make it clear for me, if I've got to use some component inside active_support I should be ready to write require 'active_support' before requiring needed component? Is the any exception or we have to do it anyway?

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Nov 14, 2013

Owner

That's right, a lib that uses Active Support has to load active_support.rb first. Same with Active Record and the rest.

Owner

fxn commented Nov 14, 2013

That's right, a lib that uses Active Support has to load active_support.rb first. Same with Active Record and the rest.

@route

This comment has been minimized.

Show comment Hide comment
@route

route Nov 14, 2013

Contributor

Thank you!

Contributor

route commented Nov 14, 2013

Thank you!

Jesterovskiy added a commit to Jesterovskiy/json-jwt that referenced this pull request Dec 22, 2014

@Jesterovskiy Jesterovskiy referenced this pull request in nov/json-jwt Dec 22, 2014

Merged

Fix issue with require ActiveSupport #19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment