-
-
Notifications
You must be signed in to change notification settings - Fork 79
Convert to v2, addon. Just wrap fetch.
#754
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
Conversation
|
|
||
| /** | ||
| * Helper function that turns the data/body of a request into a query param string. | ||
| * This is directly copied from jQuery.param. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also https://github.com/knowledgecode/jquery-param, so wondering if we should still include this in ember-fetch?
|
Todo (RFC):
Todo (This PR):
|
|
This is great! I'm getting this TS error trying out this branch: |
| }); | ||
| } | ||
|
|
||
| export default wrappedFetch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean why not just add the default to line 3 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My brain doesn't like it haha
| let original = Reflect.get(target, prop, receiver); | ||
|
|
||
| if (['json', 'text', 'arrayBuffer', 'blob', 'formData'].includes(prop)) { | ||
| return (...args) => { | ||
| let parsePromise = original(...args); | ||
|
|
||
| return waitForPromise(parsePromise); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This didn't work in Safari for our application. It turns out that's because some properties use "internal slots" for property access, so you have to wrangle the this value to be the right thing. I lifted the implementation from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy#no_private_property_forwarding
| let original = Reflect.get(target, prop, receiver); | |
| if (['json', 'text', 'arrayBuffer', 'blob', 'formData'].includes(prop)) { | |
| return (...args) => { | |
| let parsePromise = original(...args); | |
| return waitForPromise(parsePromise); | |
| }; | |
| } | |
| let original = target[prop]; | |
| if (typeof original === 'function') { | |
| return function (...args) { | |
| let maybePromise = original.apply(this === receiver ? target : this, args); | |
| // only wait for promise if it is a promise | |
| if (maybePromise && typeof maybePromise.then === 'function') { | |
| return waitForPromise(maybePromise); | |
| } | |
| return maybePromise; | |
| }; | |
| } |
Since there are other properties like toString etc which can get accessed, I made it only waitForPromise if we have a promise-like return value as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conditional use of waitForPromise makes it so it only gets called on promises. if waitForPromise is called with a non-promise, it seems to error and hang the test suite with a waiter that never resolves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rfc has it as
let parsePromise = original.call(target, ...args);which I've successfully used
|
The RFC landed on https://github.com/emberjs/ember-test-waiters/blob/master/addon/src/wait-for-fetch.ts So I'm going to close this and open a deprecation pr on the readme of ember fetch |
This PR is (what I think is) the goal.
We can decide if we want to move towards this goal incrementally or deciding that this is what we want and skip all the in-between.
To move incrementally, we'd need to resolve the following PRs:
Ember < 3.28, upgrades toEmber 4.12, supportsEmber 5#741Which... we can do -- I'm not sure it's worth keeping CI green through all that though.
ember-fetchis in a bit of disrepair.Support:
@ember/test-waitersv3+I'm not sure what the fastboot support story would be, but Node natively has
fetch,AbortController, etc.Dropped Support:
AbortControllerexists onglobalThisHeadersexists onglobalThisResponseexists onglobalThisSince
ember-auto-importtests against 3.4.0, and ember-auto-import is the only real requirement for this change, there is no change in supported ember-source.If anything, the constraining function is
@ember/test-waitersChanges:
After this PR: