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

Updates for Rails 6.0 #2

Closed
wants to merge 2 commits into from

Conversation

NickLaMuro
Copy link
Member

  • Removes usages of alias_method_chain
  • Updates calls to ActionView::Template::Handlers::RJS.call

Probably should be a major version update for this gem, but that can be up for discussion.

In Rails6, there is a deprecation warning with `.call` when working with
template handlers:

    DEPRECATION WARNING: Single arity template handlers are deprecated.
    Template handlers must now accept two parameters, the view object and the
    source for the view object.
    Change:
      >> #<ActionView::Template::Handlers::RJS:0x00007f8ecf616cb8>.call(template)
    To:
      >> #<ActionView::Template::Handlers::RJS:0x00007f8ecf616cb8>.call(template, source)

This borrows from PR 138 in Hamlit that did the same thing:

- https://github.com/k0kubun/hamlit
- https://github.com/k0kubun/hamlit/blob/master/CHANGELOG.md#293---2019-04-09
While using the `Module.prepend` approach is probably the correct way to
do this, the way that this class is defined is pretty gross, so I would
rather avoid trying to deal with that and simply replicate the
functionality `alias_method_chain` provided by doing two `alias_method`
calls.

This is a Gem the probably should just die anyway, so I am not terribly
bothered by the hack...
@NickLaMuro
Copy link
Member Author

@Fryguy and/or @bdunne can you let me know what would be needed for this to be merged and and a new version released? I am happy to assist and do the heavy lifting where needed, but this will be required for the Rails 6.0 upgrade.

Thanks!


alias_method :render_without_update, :render
Copy link
Member

Choose a reason for hiding this comment

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

Should these use prepend?

Copy link
Member Author

@NickLaMuro NickLaMuro Nov 18, 2020

Choose a reason for hiding this comment

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

@bdunne see my commit message for this:

417da52

[rails6] replace alias_method_chain

While using the Module.prepend approach is probably the correct way to
do this, the way that this class is defined is pretty gross, so I would
rather avoid trying to deal with that and simply replicate the
functionality alias_method_chain provided by doing two alias_method
calls.

This is a Gem the probably should just die anyway, so I am not terribly
bothered by the hack...

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, since this is mostly just to maintain legacy code, I am trying to do the least amount of changes possible, and using prepend is doing more refactoring than I would like, where this approach is basically taking what alias_method_chain did and doing it manually.

Fryguy
Fryguy previously approved these changes Nov 23, 2020
@Fryguy Fryguy dismissed their stale review November 23, 2020 15:19

Just noticed something.

@Fryguy
Copy link
Member

Fryguy commented Nov 23, 2020

@NickLaMuro These changes are going to the master branch, but we don't use the master branch...you'll have to put your changes on the jquery_rjs_0_1_1 branch

Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

You need to leave a comment indicating the requested changes.

:|

@NickLaMuro NickLaMuro changed the base branch from master to jquery_rjs_0_1_1 November 23, 2020 15:38
@NickLaMuro
Copy link
Member Author

@Fryguy Balls.... looks like I have to redo this then...

Anyway, will open up a new version of this PR. Thanks.

@NickLaMuro NickLaMuro closed this Nov 23, 2020
@NickLaMuro NickLaMuro deleted the rails-6 branch November 23, 2020 15:43
@NickLaMuro NickLaMuro mentioned this pull request Nov 23, 2020
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.

3 participants