Replace jquery-params with URL API and authorize undefined action name #65
Conversation
addon/utils/url-builder.js
Outdated
| let pathUrl = url.charAt(url.length - 1) === '/' ? `${url}${path}` : `${url}/${path}`; | ||
|
|
||
| return query ? `${pathUrl}?${query}` : pathUrl; | ||
| let urlObj = new URL(url, 'https://example.com'); |
There was a problem hiding this comment.
What if the url is absolute? Previously this function always returned absolute URL, while now its relative.
There was a problem hiding this comment.
Perhaps I'm the one who led us down the wrong path here, in my previous comment.
If url is always absolute something like let urlObj = new URL(url); should work, along with return urlObj.toString();
There was a problem hiding this comment.
I'm editing the PR in order to use native URL API only for queryParams and keep something more raw for the pathUrl to avoid conflict with absolute/relative URLs
Thanks for your feedbacks
GreatWizard
force-pushed
the
feat/ember-url
branch
5 times, most recently
from
8314cd7 to
44c8ada
Compare
GreatWizard
force-pushed
the
feat/ember-url
branch
from
44c8ada to
8f070c0
Compare
| @@ -1,8 +1,22 @@ | |||
| import param from 'jquery-param'; | |||
| import URL from 'url'; | |||
There was a problem hiding this comment.
Could add a note about this being a polyfill and that when Ember drop support for IE 11 this won't be needed anymore:
| } | ||
|
|
||
| // Use native URL API to generate query | ||
| let queryObj = new URL('https://exelord.com/ember-custom-actions/').searchParams; |
There was a problem hiding this comment.
| let queryObj = new URL('https://exelord.com/ember-custom-actions/').searchParams; | |
| // Passing in example.com because a domain is needed, but we're not using it. | |
| // Using `URL` only to access the queryParams API. | |
| let queryObj = new URL('https://example.com').searchParams; |
There was a problem hiding this comment.
https://host.test - never use .com it fires DNS lookup, which allows track where the request is coming from.
There was a problem hiding this comment.
Are you sure? I'm 99% certain that new URL('https://example.com') won't fire any DNS lookups. It's just a bunch of string operations.
But yeah, .test should also work well.
There was a problem hiding this comment.
In case we won't render it that's true. But just to prevent that issue in the future is better to forget about .com's for examples :)
Following #53
Hello,
This PR is here to be able to used unamed action :)
And I take a look into buildUrl mixin to replace jquery-params by the native URL API.
There is a polyfills for old browser thanks to ember-url (because URL polyfills is not handle by babel).
ping @sandstrom