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

Add Rails url_helpers #1550

Merged
merged 1 commit into from
Mar 7, 2016
Merged

Add Rails url_helpers #1550

merged 1 commit into from
Mar 7, 2016

Conversation

remear
Copy link
Member

@remear remear commented Mar 2, 2016

Purpose

Add Rails url_helpers to Link so helpers are available in the evaluation context for the block. This allows link(:link_name) { resource_url(object) }

Changes

  • Prepares Rails url_helpers and default_url_options in SerializationContext via the railtie.

Caveats

  • Rails.application.routes.default_url_options must be set as the Rails url helpers need them to build urls.

Related GitHub issues

#1454

@remear
Copy link
Member Author

remear commented Mar 2, 2016

The tests need some fixing but I'd like to start getting eyes on this.

@bf4 bf4 self-assigned this Mar 2, 2016
@bf4
Copy link
Member

bf4 commented Mar 2, 2016

💯

link :link_name, 'https://example.com/resource'
```

As a block to be evaluated. When using Rails, URL helpers are available.
Copy link
Member

Choose a reason for hiding this comment

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

  • Should also add a /howto/ for configuring the Support::UrlHelpers (after the meat of the PR is reviewed)

Copy link
Member

Choose a reason for hiding this comment

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

  • would be good to have url_for example and maybe default_url_options?

which might go in the configuration docs

Copy link
Member Author

Choose a reason for hiding this comment

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

Since having revised this PR to use SerializationContext I think we need:

  • Docs for configuring Rails.application.routes.default_url_options
  • Docs that give an example of url_for. (as previously mentioned)

Anything else?

Copy link
Member

Choose a reason for hiding this comment

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

Tests w grape adapter?

B mobile phone

On Mar 3, 2016, at 12:14 PM, Ben Mills notifications@github.com wrote:

Since having revised this PR to use SerializationContext I think we need:

Docs for configuring Rails.application.routes.default_url_options
Docs that give an example of url_for. (as previously mentioned)
Anything else?


Reply to this email directly or view it on GitHub.

Copy link
Member Author

Choose a reason for hiding this comment

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

I figured when Grape is used with Rails, the Railtie would load and SerializationContext would be available and include the url helpers. When Grape is used without Rails, the railtie would not load, SerializationContext would not be available, and no url helpers would be available. Are you wanting more of an explicit test? It doesn't look like links are even tested under Grape.

Copy link
Member

Choose a reason for hiding this comment

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

We have a grape impl, but let's not delay this pr

B mobile phone

On Mar 3, 2016, at 2:41 PM, Ben Mills notifications@github.com wrote:

In docs/general/rendering.md:

@@ -144,6 +144,32 @@ That's the result:

This feature is specific to JsonApi, so you have to use the use the JsonApi Adapter

+##### Resource-level
+
+In your serializer, define each link in one of the following methods:
+
+As a static string
+
+ruby +link :link_name, 'https://example.com/resource' +
+
+As a block to be evaluated. When using Rails, URL helpers are available.
I figured when Grape is used with Rails, the Railtie would load and SerializationContext would be available and include the url helpers. When Grape is used without Rails, the railtie would not load, SerializationContext would not be available, and no url helpers would be available. Are you wanting more of an explicit test? It doesn't look like links are even tested under Grape.


Reply to this email directly or view it on GitHub.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, sorry for a little yagni i introduced

B mobile phone

On Mar 3, 2016, at 2:41 PM, Ben Mills notifications@github.com wrote:

In docs/general/rendering.md:

@@ -144,6 +144,32 @@ That's the result:

This feature is specific to JsonApi, so you have to use the use the JsonApi Adapter

+##### Resource-level
+
+In your serializer, define each link in one of the following methods:
+
+As a static string
+
+ruby +link :link_name, 'https://example.com/resource' +
+
+As a block to be evaluated. When using Rails, URL helpers are available.
I figured when Grape is used with Rails, the Railtie would load and SerializationContext would be available and include the url helpers. When Grape is used without Rails, the railtie would not load, SerializationContext would not be available, and no url helpers would be available. Are you wanting more of an explicit test? It doesn't look like links are even tested under Grape.


Reply to this email directly or view it on GitHub.

@janvarljen
Copy link

I agree with @bf4 that SerializationContext approach would be better than including ActionMailer

@remear
Copy link
Member Author

remear commented Mar 3, 2016

I strongly prefer the SerializationContext method of handling this. Revising this PR.

remear added a commit to remear/active_model_serializers that referenced this pull request Mar 4, 2016
remear added a commit to remear/active_model_serializers that referenced this pull request Mar 4, 2016

If you wish to use Rails url helpers for link generation, e.g., `link(:resources) { resources_url }`, ensure your application sets
`Rails.application.routes.default_url_options`.

Copy link
Member

Choose a reason for hiding this comment

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

Dup text maybe instead link one to other?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know. I was thinking it would be easy to miss this in several places if it just linked to once section.

@@ -144,6 +147,33 @@ That's the result:

This feature is specific to JsonApi, so you have to use the use the [JsonApi Adapter](adapters.md#jsonapi)
Copy link
Member

Choose a reason for hiding this comment

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

jsonapi only true for resource-level, also, right?

Might it be helpful to just refer to json_api/links.md or how_to/links.md for most of the text here to keep the rendering.md slimmer and more general?

ok for followup pr

@bf4
Copy link
Member

bf4 commented Mar 7, 2016

💯 ok to merge

@mecampbellsoup
Copy link

@remear I'm wondering how the these default URL options can be set/mutated specifically in the case of PaginationLinks? Using Kaminari + AMS 10.6. Thanks and great work 😄

So far I've tried setting:

  • Rails.application.routes.default_url_options
  • ActiveModel::Serializer.default_host_options[:host] = '127.0.0.1:3000'

@remear
Copy link
Member Author

remear commented Jun 4, 2017

@mecampbellsoup I configure this from config/application.rb with the example below, which has worked well for my pagination links managed by Kaminari.

Rails.application.routes.default_url_options = {
  host: ENV['EXTERNAL_HOST'] || 'localhost:3000',
  protocol: ENV['EXTERNAL_HOST_PROTOCOL'] || 'http'
}

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

Successfully merging this pull request may close these issues.

None yet

5 participants