New Feature: Full host and protocol support #132
Conversation
…ow.location.host.
|
@andrewhavens jruby-19mode is pass on Travis, for jruby-head "Allowed Failures". About pull request: interesting idea. If @bogdan will accept it, in this case we must release new MAJOR version (maybe even 1.0.0). And notice in CHANGELOG.md about broken changes. |
|
Why we can not support |
| url_link = generate_url_link(name, route_name, required_parts) | ||
| protocol = route.defaults[:protocol] | ||
| host = route.defaults[:host] | ||
| url_link = generate_url_link(name, route_name, required_parts, protocol, host) |
There was a problem hiding this comment.
We don't need protocol and host in this method. Let's pass route.defaults[:protocol] and route.defaults[:host] directly to generate_url_link
There was a problem hiding this comment.
I would pass route directly. It seems more long term as we will want to use more and more route data when generating a link
…od. Add support for current protocol and current port.
|
@le0pard @bogdan I have updated this pull request from the feedback you gave. I restored the original specs and wrapped them in a context to indicate that they are deprecated. I moved the Let me know what you think. |
|
|
||
| before do | ||
| # V8 does not assume you want a window object, so we have to create one | ||
| jscontext['window'] = {:location => {:protocol => current_protocol, :hostname => current_hostname, :port => current_port, :host => current_host}} |
There was a problem hiding this comment.
I think this one should be moved in context, which really need it (to test window call). Also should be tests, which show what without window object generated js file still have working code (because js-routes must continue work on server side js engines also).
There was a problem hiding this comment.
@le0pard What do you mean by "moved in context"? It is already in a context. Do you mean that the tests should be refactored so that only the tests that generate URLs based on window should set the window object?
What should be the behavior when window is not defined? Can you provide an example use-case which requires this support? I don't know of a situation where we would need to support that. As far as I understand, this gem is only intended to work with a Rails app.
There was a problem hiding this comment.
What do you mean by "moved in context"? It is already in a context. Do you mean that the tests should be refactored so that only the tests that generate URLs based on window should set the window object?
This should be in context where we testing window behaviour. All another contexts should be without this defined object.
Can you provide an example use-case which requires this support? I don't know of a situation where we would need to support that.
- Node.js
- Therubyracer
- Browserify ( Custom root option #121 )
And many other I can find :)
As far as I understand, this gem is only intended to work with a Rails app.
Gem is generate JS code with Rails routes. This mean, what this generated JS code can use not only browsers and Rails app.
Look how we works with jQuery inside js-routes. We don't depend from it and can work without it, but if jquery exists - we use it functionality.
Also, compatibility with CommonJS shouldn't be broken by another feature.
There was a problem hiding this comment.
@le0pard Right. I understand. You want to be able to reference your Rails routes in your Node.js application. To be clear, this code will not break if you configure a default protocol, host, and port. I'm not sure what you would like me to do if these defaults aren't defined. Should I leave them blank? Should I console.log or raise an alert?
I can add checks to see if window.location is defined, otherwise return an empty string (same as *_path helpers). The result will look something like this:
(typeof window !== 'undefined' && typeof window.location !== 'undefined' && window.location.protocol != '' ? window.location.protocol + '//' : '') + (typeof window !== 'undefined' && typeof window.location !== 'undefined' && window.location.hostname != '' ? window.location.hostname : '') + (typeof window !== 'undefined' && typeof window.location !== 'undefined' && window.location.port != '' ? ':' + window.location.port : '')This seems unnecessarily complex to support this feature, while still trying to avoid unecessary concatenation and trying to keep it to one line. Rather than chaining all these ternary operators, and duplicating the "undefined" check, I can add additional JavaScript logic within the URL helper function to DRY up the code, but that will result in more lines. Personally, I feel that the code should be written to be clean and maintainable while supporting the 95% use-case. For the less common (5%) scenarios, the responsibility is on the developer to read the documentation and configure it properly.
There was a problem hiding this comment.
I understand, but js should be valid for all possible cases. Maybe you can just remove window.location from code.
There was a problem hiding this comment.
I'm willing to split the window.location code into a separate pull request if that means this pull request can be approved sooner. I still think having a window.location fallback is very useful. However, this doesn't solve the problem because the question still exists...what should happen if the developer does not configure a default protocol/host/port?
There was a problem hiding this comment.
For url_links it was simple: if you defined it (in valid format) - you will have url links, if no - no url links. I think for your case possible the same, but for host: not defined - no url links. Like this:
unless @options[:url_links] == true
...
elsif host
...
else
nil
end
This is an alternative implementation of #131 which is more full featured.
New Features:
default_url_options.default_url_options.default_url_options.window.location.protocolwhen a protocol has not been specified.window.location.hostwhen a host has not been specified.window.location.portwhen a port has not been specified.Changes:
generate_url_linkno longer validates that the:url_linksconfig value contains a protocol.url_linksto boolean true/false and deprecates the previous usage. Default protocol, host, and port can be configured withdefault_url_options.This PR fixes #127.