[BUGFIX #4497] query/queryRecord/filter now support adapter options #4856
Conversation
|
Thanks! Reviewing now. |
addon/-private/system/store.js
Outdated
| let options = {}; | ||
|
|
||
| options.adapterOptions = query.adapterOptions; | ||
| delete query.adapterOptions; |
There was a problem hiding this comment.
should we test that this is removed from query?
There was a problem hiding this comment.
or is this already handled by an existing test?
addon/-private/system/store.js
Outdated
| let options = {}; | ||
|
|
||
| options.adapterOptions = query.adapterOptions; | ||
| delete query.adapterOptions; |
There was a problem hiding this comment.
should we test that this is removed from query?
stefanpenner
force-pushed
the
query-adapter-options
branch
from
b81f6f6
to
c2783fb
Compare
|
we may want to consider moving |
👍 |
runspired
force-pushed
the
query-adapter-options
branch
from
c2783fb
to
0152014
Compare
|
@hjdivad and I discussed this OOB and decided that moving to a new optional last param for This has weirdness for |
|
Test failures on ember-canary are unrelated, it appears we have some breaking change to dig into unless there's a bug in Ember. |
|
👯♂️ |
| @@ -143,14 +143,14 @@ export function _query(adapter, store, modelName, query, recordArray) { | |||
|
|
|||
| if (createRecordArray) { | |||
| recordArray = recordArray || store.recordArrayManager.createAdapterPopulatedRecordArray(modelName, query); | |||
| promise = Promise.resolve().then(() => adapter.query(store, modelClass, query, recordArray)); | |||
| promise = Promise.resolve().then(() => adapter.query(store, modelClass, query, recordArray, options)); | |||
| } else { | |||
| promise = Promise.resolve().then(() => adapter.query(store, modelClass, query)); | |||
There was a problem hiding this comment.
Was it intentional that options isn't passed along if createRecordArray is false? There isn't such a distinction in the documentation about when an included adapterOptions hash will be passed to adapter.query.
There was a problem hiding this comment.
I think it was a mistake actually.
@runspired / @hjdivad / @igorT can you sanity check that?
There was a problem hiding this comment.
Speaking with @runspired he reminded me what this was intentionally, as without a record array, we cannot pass options as a fourth argument without breaking user-land arity checks. Also, creating the record array eagerly and providing it here, had some performance implications.
The plan at the time, was to eventually change this API to use an object, instead of positional arguments, to avoid this problem.
There was a problem hiding this comment.
Additional note: if the arity check says that adapter.query uses less than 4 arguments (recordArray is 4th, options is 5th), we don't pass either along.
I'm not sure we were as worried about breaking user-land arity checks as we were about the performance costs of materializing the snapshotRecordArray when we didn't need to.
If your query implementation has an arity of 4/5 and is still not receiving options, then let's dive into that; however, simply adding it into the function signature in prep for usage should be enough to receive any options that were passed in.
There was a problem hiding this comment.
@runspired and I dug into this more, and there's an issue with the arity check where it doesn't properly detect the argument length for adapter.query. This is captured in #6232.
[fixes #4497]