Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX beta] Fixes issue with GET requests appending ?{} to url #4445

Closed

Conversation

JeroenvdV
Copy link

This fixes a typo in the REST adapter comparing request.type, which doesn't exist. Either request.method or hash.type was meant. The symptom is that ?{} is appended to the url of any GET requests by jQuery.

@@ -1341,7 +1341,7 @@ if (isEnabled('ds-improved-ajax')) {
hash.context = this;

if (request.data) {
if (request.type !== 'GET') {
if (hash.type !== 'GET') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this to be request.method instead?

@pangratz
Copy link
Member

pangratz commented Jun 24, 2016

Thanks for this @JeroenvdV. I think it would be good to have a test to catch future regressions for this.

Can you add a test to the rest-adapter-test.js which looks something like this:

if (isEnabled('ds-improved-ajax')) {

  test("_requestToJQueryAjaxHash works correctly for GET requests - GH-4445", function(assert) {
    let done = assert.async();
    let server = new Pretender();

    server.get('/posts/1', function(request) {
      assert.equal(request.url, "/posts/1", "no query param is added to the GET request");

      return [201, { "Content-Type": "application/json" }, JSON.stringify({ post: { id: 1 } })];
    });

    run(function() {
      let post = store.findRecord('post', 1);

      post.then(function() {
        server.shutdown();
        done();
      });
    });
  });

}

After this, this is good to go 🚀

@JeroenvdV
Copy link
Author

JeroenvdV commented Jun 24, 2016

Thanks, I added your test verbatim as I'm not in a position to test it fully at the moment.

@pangratz
Copy link
Member

Just one more tiny change: if you add a newline at the end of the test then ESLint is happy and this is good to be merged!

@pangratz
Copy link
Member

@JeroenvdV, I should have been more clear: I meant adding a newline at the end of the test file. I am very sorry for the back and forth in this PR, I can see how that is not very motivating and I try to communicate more clearly in the future 😔 ...

If you don't have time for this today, I can add the newline later manually...

@pangratz
Copy link
Member

pangratz commented Jul 7, 2016

Closing in favor of rebased version #4466

@pangratz pangratz closed this Jul 7, 2016
@pangratz
Copy link
Member

pangratz commented Jul 7, 2016

@JeroenvdV I merged this via the rebased #4466. Thank you very much! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants