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

refactor(jQuery): Remove $.ajax, use async/await, move custom paths t… #1206

Conversation

geekygrappler
Copy link
Collaborator

…o their own test

  • I wanted to remove $.ajax, so replaced it with fetch
  • Used async/await hopefully makes the tests easier to follow
  • We were testing custom paths using posts model in every test, moved testing of this behaviour into it's own test and removed from all other tests.

@geekygrappler
Copy link
Collaborator Author

It was bugging me that we had $.ajax in there and were not using async/await, so I fixed it for this file. It does introduce ember-fetch though, maybe you don't want that dependency @samselikoff?

…o their own test

- I wanted to remove $.ajax, so replaced it with fetch
- Used async/await hopefully makes the tests easier to follow
- We were testing custom paths using `posts` model in every test, moved testing of this behaviour into it's own test and removed from all other tests.
@rwjblue
Copy link
Collaborator

rwjblue commented Nov 5, 2017

This seems good to me, but generally speaking the only reason ember-fetch works is because it still uses XMLHttpRequest under the hook (e.g. it is not a "true" polyfill for native fetch). We need to create a pretender adapter for fetch (I think @cibernox had a spike of this at one point).

I'm slightly concerned about landing these changes considering that I am also pushing to use native fetch if the host apps targets support it.

Some background links:

@cibernox
Copy link
Collaborator

cibernox commented Nov 5, 2017

I did. I can't locate it, but it's nothing I can't reproduce within a couple hours. The web community in general, not only the Ember community, needs a fetch stub that works.

@geekygrappler
Copy link
Collaborator Author

Thanks for taking a look @rwjblue.

I am also pushing to use native fetch if the host apps targets support it.

Does that require some work on Pretender first to intercept native fetch requests?

From reading the links, I guess my only concern now would be that ember-fetch stops using XMLHttpRequest, and uses native fetch which would mean these tests break... I think.

In any case I think I'll split this into 3 different PRs. I think we can get the async/await changes and the moving of the custom paths merged fairly easily. The fetch stuff can wait.

@rwjblue
Copy link
Collaborator

rwjblue commented Nov 6, 2017

Sounds good, thanks for working on this!

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.

3 participants