Add support for host, protocol, and port configuration.

[andrewhavens]

This PR is an alternative to #131, #132, and fixes #127.

New Features:

• Adds support for specifying a default protocol, host, and port in default_url_options.
• Adds support for routes which specify a specific protocol, host, or port.

Changes:

• Changes expectation of url_links to a boolean value and deprecates the previous usage. Default protocol, host, and port are now configured with default_url_options.

[andrewhavens]

@le0pard @bogdan With this pull request, I started over from scratch, just implementing the basic necessary features while keeping support for server-side JavaScript usage. Please review.

[bogdan]

Why you can not use routes.sso_url as expected value?

[andrewhavens]

We can do that. I will update it.

[le0pard]

Looks good! 👍

[bogdan]

what is the difference between this context and when default host is specified ?

[andrewhavens]

This context is for testing the effects of not specifying a protocol, while the previous context is for testing the effects of not specifying a host. Technically, the options specified in these contexts are the same because a host is required, but the tests within these contexts are specific to the thing that they are testing.

[bogdan]

I treat context as in which context the API function was called rather than which function from API was called. So for me context is same and described like this:

let(:_options) { { :url_links => true, :default_url_options => {:host => "example.com"} } }

[andrewhavens]

I think about it the same way, but from the user's perspective of configuring the application, and organize my tests based on their action and validate the outcome.

Are you advocating for something that looks like this?

      context "when default host is not specified" do
        it "raises an error" do
          expect { JsRoutes.generate({ :url_links => true }) }.to raise_error RuntimeError
        end
      end

      context "when only host option is specified" do
        let(:_options) { { :url_links => true, :default_url_options => {:host => "example.com"} } }

        it "uses the specified host, defaults protocol to http, defaults port to 80 (leaving it blank)" do
          expect(evaljs("Routes.inbox_url(1)")).to eq("http://example.com#{routes.inbox_path(1)}")
        end

        it "does not override protocol when specified in route" do
          expect(evaljs("Routes.new_session_url()")).to eq("https://example.com#{routes.new_session_path}")
        end

        it "does not override host when specified in route" do
          expect(evaljs("Routes.sso_url()")).to eq(routes.sso_url)
        end

        it "does not override port when specified in route" do
          expect(evaljs("Routes.portals_url()")).to eq("http://example.com:8080#{routes.portals_path}")
        end
      end

      context "when default host and protocol are specified" do
        let(:_options) { { :url_links => true, :default_url_options => {:host => "example.com", :protocol => "ftp"} } }

        it "uses the specified protocol and host, defaults port to 80 (leaving it blank)" do
          expect(evaljs("Routes.inbox_url(1)")).to eq("ftp://example.com#{routes.inbox_path(1)}")
        end

        it "does not override protocol when specified in route" do
          expect(evaljs("Routes.new_session_url()")).to eq("https://example.com#{routes.new_session_path}")
        end

        it "does not override host, protocol, or port when host is specified in route" do
          expect(evaljs("Routes.sso_url()")).to eq(routes.sso_url)
        end

        it "does not override port when specified in route" do
          expect(evaljs("Routes.portals_url()")).to eq("http://example.com:8080#{routes.portals_path}")
        end
      end

      context "when default host and port are specified" do
        let(:_options) { { :url_links => true, :default_url_options => {:host => "example.com", :port => 3000} } }

        it "uses the specified host and port, defaults protocol to http" do
          expect(evaljs("Routes.inbox_url(1)")).to eq("http://example.com:3000#{routes.inbox_path(1)}")
        end

        it "does not override protocol when specified in route" do
          expect(evaljs("Routes.new_session_url()")).to eq("https://example.com#{routes.new_session_path}")
        end

        it "does not override host, protocol, or port when host is specified in route" do
          expect(evaljs("Routes.sso_url()")).to eq(routes.sso_url)
        end

        it "does not override port when specified in route" do
          expect(evaljs("Routes.portals_url()")).to eq("http://example.com:8080#{routes.portals_path}")
        end
      end

Personally, I feel that this approach to organization makes the tests harder to read and maintain. It also requires some duplication of tests which is less desirable.

However, if this is how you would like it to read, I will make it so. I'm more concerned with getting this merged and released. =]

[bogdan]

Yes, please make it so.

For if this is taking too long to merge, we are introducing major changes that is never easy and better to eliminate all problems with tested behaviour. Implementation can suck, but every test should clearly reviewed so that bad behaviour never becomes a standard.

[andrewhavens]

@bogdan I have updated the tests. Please review.

[bogdan]

What do you think about configuring Rails with default_url_options during doing tests so that we always use rails _url helper as expected value on the right side? I think only in this way we can get things consistent.

[andrewhavens]

@bogdan I think that might be a good long-term goal. However, I think there are some reasons why we would not want to do that right now. js-routes provides its own configuration of default_url_options so the routes that are generated, so the default values may be different than those specified in the application. Specifying default_url_options in Rails would override the values specified in js-routes config. Maybe a long-term goal should be to remove the js-routes default_url_options config and instead rely on Rails.application.routes.default_url_options.

I think the tests are good right now because they explicitly show the expected value, rather than obscuring it in a variable.

[bogdan]

I remember we were talking about default port to be window.location.port. Why did we decide to skip it?

[andrewhavens]

@bogdan I implemented that feature in #132, but @le0pard mentioned that referencing window.location would break server-side usage, so I created this new pull request which does not include any window.location defaults. I'm planning to submit another pull request (after this one is merged) which reintroduces using window.location for defaults.

[bogdan]

We need to prevent warnings in the test suite:

http://monosnap.com/image/Rx528u4Z05jJiPjaLX0iaVFDbJrmMr.png

[andrewhavens]

@bogdan I have added a line to spec_helper.rb which silences the deprecation warnings.

[andrewhavens]

@bogdan @le0pard Can we approve and merge this today?

[le0pard]

@andrewhavens I can merge it, but main contributor is @bogdan and he always has the final decision.

[bogdan]

It is bad to just silence all warnings for entire suite. We will not know when we will use rails deprecated calls.

We need to disable warnings only for tests that produce them.

[andrewhavens]

I agree. How do you suggest that we achieve that?

On Monday, Dec 15, 2014 at 5:33 PM, Bogdan Gusiev notifications@github.com, wrote:

It is bad to just silence all warnings for entire suite. We will not know when we will use rails deprecated calls.

We need to disable warnings only for tests that produce them.

—
Reply to this email directly or view it on GitHub.

[bogdan]

From activesupport docs:

      # Silence deprecation warnings within the block.
      #
      #   ActiveSupport::Deprecation.warn('something broke!')
      #   # => "DEPRECATION WARNING: something broke! (called from your_code.rb:1)"
      #
      #   ActiveSupport::Deprecation.silence do
      #     ActiveSupport::Deprecation.warn('something broke!')
      #   end
      #   # => nil

around(:each) do |example|
  ActiveSupport::Deprecation.silence do
    example.run
  end
end

[andrewhavens]

@bogdan I have moved the deprecation silencer to a block, as you have recommended, and applied it to only the specs that generate deprecation warnings. Please review

[bogdan]

Merged.

Thanks everyone for hard work and patience. Especially @andrewhavens. This one was really hard.

[le0pard]

👍 🆒

Now my pull request: #135