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

Update the Kernel#require comment about prepend #203

Merged
merged 1 commit into from
Jan 12, 2022

Conversation

casperisfine
Copy link
Contributor

Ref: Shopify/bootsnap#389

I wondered the same, and was surprised to see it work. Turns out it was fixed recently.

Ref: Shopify/bootsnap#389

I wondered the same, and was surprised to see it work.
Turns out it was fixed recently.
@fxn
Copy link
Owner

fxn commented Jan 12, 2022

Ooohhh, that's excellent!

module M
end

class C
  include M
end

module N
  def x
    :x
  end
end

module M
  include N
end

p C.new.x

Now that works!

Then, I might refactor this decoration, I prefer prepend + super so that you see the decoration in the ancestor chain.

@fxn fxn merged commit 960dbdc into fxn:main Jan 12, 2022
@casperisfine
Copy link
Contributor Author

Then, I might refactor this decoration, I prefer prepend + super so that you see the decoration in the ancestor chain.

Me too, but 3.0 is a bit tricky, it likely mean feature testing and doing one or the other. I'm tempted to do it in Bootsnap. If you end up doing it please let me know.

@fxn
Copy link
Owner

fxn commented Jan 12, 2022

3.0 is a bit tricky

In what sense?

I'm tempted to do it in Bootsnap. If you end up doing it please let me know.

Right now, RubyGems, Bundler, Bootsnap, and Zeitwerk alias require. You just adjusted Bootsnap, and if this proposal ends up implemented, the four of them will cooperate quite well, I believe.

That reduces a sense of urgency, and if things work well in practice, I'd need to think if having two decorations is worthwhile. Need to have it written to see it.

Dropping support for Ruby < 3.0 some day would be more clear.

@casperisfine
Copy link
Contributor Author

3.0 is a bit tricky

In what sense?

I meant it means either dropping 2.7 support or doing the work twice. I absolutely don't mind the later, but it means the gems behaves a bit differently on different version, so we'd have to be careful to see how it works with Rubygems and such.

For context I'd like to do this because I'd want to be able to remove Bootsnap hooks post boot, but it's close to impossible to do correctly with alias method chain.

if this proposal ends up implemented

Oh interesting, subscribed!

@fxn
Copy link
Owner

fxn commented Apr 24, 2022

I was trying this refactor, and one test failed, because aliasing in Kernel after my decoration had no effect.

Bottom line, if you alias the original method after prepend, the alias is ignored:

module M
  def f
    p :original
  end
end

class C
  include M
end

module N
  def f
    p :prepended
    super
  end
end

M.prepend(N)

C.new.f

module M
  alias_method :original_f, :f

  def f
    p :aliased
    original_f
  end
end

C.new.f

that prints

:prepended
:original
:prepended
:original

No :aliased printed.

Even more weird, if you comment out the first C.new.f and leave only the bottom one, the script raises:

`f': super: no superclass method `f' for #<C:0x00000001100fc880> (NoMethodError)

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.

None yet

3 participants