search

imgix / ember-cli-imgix

Easily add imgix functionality to your Ember application
https://imgix.github.io/ember-cli-imgix
MIT License
26 stars 13 forks source link

Update URL parsing to work in older browsers (e.g. IE 11) #41

Closed Duder-onomy closed 6 years ago

Duder-onomy commented 6 years ago

Right now, we support all the browsers that the latest ember version supports. This file outlines the versions that ember core auto-runs on browserstack.

Specifically, we use window.Url to parse the url and its params. This api is not available on older browsers (Safari 9.1.2 from July 2016).

My question is, How far back should we support?

My feeling is that, We should support all browsers and browser versions that the latest ember supports.

If we support only what the latest ember supports then the use of the window.URL.searchParams api is totally legit... and this can be closed. We should probably add a note to the readme, something like "We support the browsers that the latest Ember supports"

If we DO want to support older browser that the latest ember does not technically support (but actually do work), We should use something like this: https://github.com/defunctzombie/node-url ^ We might be able to find something smaller as we only need the param parsing. ^ Would be a near trivial import using https://github.com/ef4/ember-auto-import ^ We could also use that lib in fastboot if we wanted to add better fastboot support.

Here is a screenshot of that api failing on safari 9.1.2 (over 2 years old) image

This update will be trivial and I need to do the work anyways as TheDyrt's users seem to have old computers.

Thoughts?

frederickfogerty commented 6 years ago

Hey @Duder-onomy. Thanks for the detailed report. My hunch is the same as yours - that we should only support the browsers that the latest version of ember supports.

I think it's dangerous to say that we support browsers that ember doesn't support because there could be non-obvious, awkward edge cases that may be hard to handle.

I would be against using node-url as a) it's another dependency, and we should try to limit dependencies, and b) it's not a very large package (only has 100 stars on github).

In the past we've used uri.js to handle URLs, so if we had to add a dependency back, I'd like to use uri.js over node-url as it has more stars and is more supported.

Overall, my first impression is that we should leave the code how it is now. How does this affect your work?

frederickfogerty commented 6 years ago

In saying this, I've just found that window.URL is not supported on IE 11 - so we should probably fix this.

Looking at node-url again, it does have quite a lot of downloads, so I'm more happy to use it now. I'd be wary of it potentially not working in the browser, so I would also like to know how it stacks up against https://github.com/github/url-polyfill. According to the README, it "has been converted to pragmatic JS with a huge performance improvement as a side effect."

Duder-onomy commented 6 years ago

After talking with my team over here, I think this is worth fixing. Additionally, I would like to add this in there: https://github.com/imgix/ember-cli-imgix/issues/40 We will need a Url parsing mechanism during fastboot in order to pull that ^ off.

I guess the only question now is which lib should we use? I like node-url only because I trust that author (a heavy hitter from the node community) But honestly, all we need to do with this lib is params parsing... Does https://github.com/imgix/imgix-core-js Have url parsing that we could easily expose? How are people handling this in React land?

frederickfogerty commented 6 years ago

Agreed. This should be fixed.

node-url looks to be well supported, but url-polyfill comes from the Web Components team, who are also heavy hitters in the web space.

imgix-core-js doesn't have anything that can be exposed, and is also not supported in the browser. In React we use jsuri to handle query parameters.

Re #40: We are planning to implement responsiveness using srcset and sizes soon (#39), which will probably interfere with #40. We'd love your thoughts on using srcset etc over there if you have any.

Duder-onomy commented 6 years ago

Rad. I will work on this tomorrow morning and PR shortly after. Lets do url-polyfill, the weekly downloads are insane === probably pretty good? Thanks for the quick feedback.

Duder-onomy commented 6 years ago

Aight. As I work on this, some browsers will need a polyfill for the basic implementation of window.URL, and some, like old safari, only need a polyfill for the searchParams portion of the spec. This lib url-polyfill does not polyfill the searchParams portion. Only the basic window.URL api.

So, I can use something like this, which seems to polyfill the entire spec: https://github.com/lifaon74/url-polyfill

OR we can use a completely diff implementation like jsuri

My gut, and what Im gonna go with unless I head otherwise, is to use the polyfill as, eventually 🤞... we can gut rip out that polyfill.

Duder-onomy commented 6 years ago

Ok, lol. Gonna scrap that polyfill and just switch to jsuri We need this functionality: https://github.com/lifaon74/url-polyfill/issues/27

frederickfogerty commented 6 years ago

I'm with you on your gut instinct which is to use a polyfill.

Do either of these polyfills work? https://github.com/WebReflection/url-search-params, https://github.com/jerrybendy/url-search-params-polyfill. Both have ~40k weekly downloads.

Duder-onomy commented 6 years ago

Yeah, but then we will need to manually manage the synchronization of the searchParams and the top level search property.

In a working full implementation of window.URL the searchParams property, when modified, will reflect its modified values in the search property. We will have to manage that if we polyfill them separately.

FWIW... Switching to jsuri was a breeze and is almost done... All tests pass.

Duder-onomy commented 6 years ago

Actually, I am slightly confused now after reading through the react implementation. It seems as though the final URL is never passed to the imgix core lib, only parsed by jsuri then shoved into a src set.

Is imix-core even useful in this case?

frederickfogerty commented 6 years ago

The react implementation doesn't use imgix-core-js, but I'm not sure this is a 'feature' - it's just the state it's in right now. Since we're currently using it, we should probably keep using it. React will probably end up using it in the future.

frederickfogerty commented 6 years ago

Just a quick question, why/where do we need the two way binding? Can we work around to not need two way binding?

Duder-onomy commented 6 years ago

Is there any advantage to using imgix-core? I have everything ready to go, but I have to serialize, then deserialize params in order to pass everything to imgix-core-js correctly. We could clean up some logic if we dropped it. Imgix-core-js will always prepend the host to image paths even if the path already has a host. We should probably rip it out, or teach it how to accept a path with a host already on it.