Make more resilient about adding query parameters at levels below Ember

[YoranBrondsema]

In our application we dynamically add query parameters for authentication through $.ajaxSetup. In that case, settings.url contains something like /model_type/123?auth=xxx so the basic match fails. We made this more flexible by only comparing the path of settings.url, i.e. ignoring any query parameters. I don't think this causes a problem as query parameters are checked at the ember-data-factory-guy level further on in extraRequestMatches.

The only downside is that it requires an additional dependency on urijs. However, as this addon is only used in a testing environment and none of the code will ever make it to a production environment, I don't think it's a huge deal.

[danielspaniel]

I reckon your right, this is a more foolproof way to check the url matches. Could you add a test for this just for fun. You can put it in mock-request-tests.js .. should be simiple, and will help to remember why this was added.

[YoranBrondsema]

@danielspaniel Cool, I added some tests. I also cleaned up that test file a bit and updated outdated packages. Let me know if it's all good for you.

[danielspaniel]

Is this making a vendor file out of the npm file?
Can you look at issue: #206
and see if you can hook that up somehow?
I am trying to take the tests/factories directory and include those files in the development build ( don't really care where .. as in vendor.js , or the app ( appName.js ) or even a separate js file that I can then include the way you did here
( after you made the file ) .. as long as those factory files are watched and updated when changed I would be your best friend for a day if you could make that work.

[YoranBrondsema]

Yes, this is including the urijs addon and including it in the build. I took inspiration from other addons https://github.com/bustlelabs/ember-mobiledoc-editor/blob/master/index.js and https://github.com/ember-animation/liquid-fire/blob/master/index.js.

Are you sure the factories are not already included in the build? If I run ember s and go to localhost:4200/tests then I can use all my factories and functions exported by ember-data-factory-guy.

[danielspaniel]

Yes they are included in the build .. they are put into tests.js
and that file is added when tests/index.html includes it as a script tag.
And it would be simple to just include that too in the app/index.html file BUT
that tests.js file also has the test-helper.js file which requires ember-qunit ( and that is not available to development build .. not sure why?? but the easy answer would be to include the tests.js in the contentFor 'bodyFooter' and then figure out a way to make ember-qunit available to development .. just not sure how to do that
or even better would be to make a file like tests.js but that does not have the tests and the test-helper and include that in the contentFor 'bodyFooter'

[YoranBrondsema]

OK I see what you mean now. I'll try to find some time to play around with the issue and come to a solution.

[danielspaniel]

Super .. glad you can give it a look. I need the help.
I am going to try again tonight, after failing for last few days over and over again. Your tip about those other repos was helpful.

[danielspaniel]

Interestingly .. just yesterday I came upon a use case for this update to checking just the uri .. and I had thought it was never going to be much of an issue .. so this is timely.

[danielspaniel]

Yoran. I have something working for loading factories in developement. Your orignial idea and mine ( the one I tried ) was actually pretty close to target. But I am all set now.

And in testing out things, the only thing that broke was the

  basicRequestMatches(settings) {
    const uri = new URI(settings.url);
    return uri.path() === this.getUrl() && settings.type === this.getType();
  }

concept you added here about checking the URI .. but was no big deal because this fixed it.

  basicRequestMatches(settings) {
    return URI(settings.url).equals(this.getUrl()) && settings.type === this.getType();
  }

seems like in test the url that is built does not include the whole http://blah blah .. but in development it does. So by comparing by uri for both .. it ignores the host name completely .. which I think is great, but do you think there is a flaw here ? Am I missing a case where people might want to match on host name. I can't think of it .. but ??

[danielspaniel]

Also .. your updates to the bower and npm packages for qunit and all else got the tests running .. oh 10 times faster .. amazing .. I used to have to fall asleep and wait for them to finish. No more! 👍

[YoranBrondsema]

Yoran. I have something working for loading factories in developement. Your orignial idea and mine ( the one I tried ) was actually pretty close to target. But I am all set now.

And in testing out things, the only thing that broke was the

basicRequestMatches(settings) {
const uri = new URI(settings.url);
return uri.path() === this.getUrl() && settings.type === this.getType();
}

concept you added here about checking the URI .. but was no big deal because this fixed it.

basicRequestMatches(settings) {
return URI(settings.url).equals(this.getUrl()) && settings.type === this.getType();
}

seems like in test the url that is built does not include the whole http://blah blah .. but in development it does. So by comparing by uri for both .. it ignores the host name completely .. which I think is great, but do you think there is a flaw here ? Am I missing a case where people might want to match on host name. I can't think of it .. but ??

So if I understand correctly, this.getUrl() returns the full URL in development, not just the path? I think it's OK to be stricter and include the host name. I can't think of a scenario myself either but then again our imagination is limited :-).

Also .. your updates to the bower and npm packages for qunit and all else got the tests running .. oh 10 times faster .. amazing .. I used to have to fall asleep and wait for them to finish. No more! 👍

Low-effort high-impact changes are the best :-)