Cropster / ember-l10n

A GNU gettext based localization workflow for Ember
MIT License
15 stars 7 forks source link

Drop ember-ajax dependency #50

Closed mydea closed 5 years ago

mydea commented 5 years ago

Currently, we use ember-ajax to fetch our locale files. This uses $.ajax() under the hood, which is not really ideal. This PR switches this with ember-ajax instead, which is just a polyfill for fetch, and thus does not depend on jQuery.

The actual change here is really small, most of the changes are for the tests, where I replaced the ajax service mock with pretender based mocks.

I guess this can be considered a breaking change, even though it shouldn't actually break for most people I guess - as somebody might have overwritten/extended the l10n-ajax service.

mydea commented 5 years ago

Hmm, it fails because of ember-try and jQuery - I'll investigate.

mydea commented 5 years ago

I removed the commit to disable jQuery, as we can tackle this at another time. Should be ready to review now!

If this looks fine to you @arm1n , I would propose to make a new major release with this & the AST stuff in it.

arm1n commented 5 years ago

So we are replacing a dependency with another one to do this 5 LOC:

var request = new XMLHttpRequest();
request.open("GET", "/path/to/lang.json");
request.addEventListener("load", onSuccess);
request.addEventListener("error", onFailure);
request.send();

This is a typical example of where a dependency makes no sense IMHO and introduces nothing else but complexity and maintenance costs. We used ember-ajax, now for some reason we can't use it anymore (fun fact: because it uses other dependencies we don't want anymore), and then we are going to do the same again to perform one simple GET request? I don't think that's reasonable, do you?

mydea commented 5 years ago

Well, the difference is that ember-fetch is only a polyfill, so if you don't need to support IE11, it introduces no overhead. Also, in contrast to ember-ajax it does not force you to include jQuery, which is already a big win.

I think the main issue with plain XMLHttpRequest is that it does not work in non-browser environments, and thus in Fastboot.

arm1n commented 5 years ago

Hm, so we indirectly depend on the implementation details of ember-fastboot? Not sure if I like that - software should be loosely coupled, and if addons depend on other addons something went wrong...

Looking into ember-fetch docs, it requires adaptions to disable fastboot's ajax layer, whereas in the official fastboot docs they suggest to use ember-fetch - I don't understand this. Additionally, it requires ember-fetch to be a peerDependency, thus again bubbling up to host's dependencies. And last but (definitely) not least it will introduce a new bunch of problems, f.e. here.

I really don't like all these things. If somebody wants to use our addon with ember fastboot, he (or she) simply has to override the corresponding ajax service from our lib - no deps, no coupling, no worries...

mydea commented 5 years ago

OK, so I changed it to use XMLHttpRequest instead of fetch!