Improve fetch #6155
Improve fetch #6155
Conversation
1. Don't ever assign the internal `fetch` to `null`. Instead, throw immediately if fetch is not found. 2. Don't extract the global fetch into module state. Instead, export a function that looks up the global fetch each time. This allows libraries that mock `fetch` to do so after Ember Data has already been loaded.
There was a problem hiding this comment.
Could you roll back the
todorelated changes? As far as I can tell they are unrelated to fixing thefetchissue, and they will cause the test suite to fail becausetestemdoes not support usingQUnit.todo's.
Sure. That's why we separated into a different commit :)
|
Done. |
| _fetch = () => fetch; | ||
| } else { | ||
| throw new Error( | ||
| 'cannot find the `fetch` module or the `fetch` global. Did you mean to install the `ember-fetch` addon?' |
There was a problem hiding this comment.
Nice touch, thank you for adding this!
| return fetchFunction(options.url, options); | ||
| } else { | ||
| throw new Error( | ||
| 'cannot find the `fetch` module or the `fetch` global. Did you mean to install the `ember-fetch` addon?' |
There was a problem hiding this comment.
Do we need the same error in both places? What does that buy us?
There was a problem hiding this comment.
I wasn't sure which of the two we should do. At first glance, I'd prefer it to happen at the top level.
|
I could be misremembering but isn't stubbing ES modules in this way not possible in native and only possible due to an artifact of transpilation? If so, are we sure we want to encourage this pattern vs finding another way to provide the ability to stub? |
|
@runspired AFAIK, no one is stubbing an ES module. Pretender does |
1. Don't ever assign the internal `fetch` to `null`. Instead, throw immediately if fetch is not found. 2. Don't extract the global fetch into module state. Instead, export a function that looks up the global fetch each time. This allows libraries that mock `fetch` to do so after Ember Data has already been loaded.
We're not sure how to test this change. We'd welcome any advice 😄 (@rwjblue?)
We also added some errors for the situation where
fetchcould not be found as a module or on the global. We weren't sure where people would prefer to put this error (and whether it would be a problem to throw during module evaluation). We're open to any feedback on this 😄