-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: coalesceFindRequests should work with non native Adapter classes #7448
Conversation
Performance Report for 90b02d8 Relationship Analysis
|
Asset Size Report for 90b02d8 IE11 Builds The size of the library EmberData has increased by 438.0 B (9.0 B compressed) which exceeds the failure threshold of 75 bytes.WarningsThe uncompressed size of the package @ember-data/adapter has increased by 438.0 B. Changeset
Full Asset Analysis (IE11)
Modern Builds The size of the library EmberData has increased by 438.0 B (9.0 B compressed) which exceeds the failure threshold of 75 bytes.WarningsThe uncompressed size of the package @ember-data/adapter has increased by 438.0 B. Changeset
Full Asset Analysis (Modern)
Modern Builds (No Rollup) EmberData increased by 438.0 B uncompressed but decreased by 52.0 B compressedWarningsThe uncompressed size of the package @ember-data/adapter has increased by 438.0 B. Changeset
Full Asset Analysis (Modern)
|
packages/adapter/addon/index.ts
Outdated
@@ -470,6 +470,8 @@ export default class Adapter extends EmberObject implements MinimumAdapterInterf | |||
return RSVPPromise.resolve(); | |||
} | |||
|
|||
_coalesceFindRequests = true; |
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.
We could use #
private fields here but that will throw an error with non native classes
packages/adapter/addon/json-api.ts
Outdated
@@ -231,7 +231,8 @@ class JSONAPIAdapter extends RESTAdapter { | |||
@property coalesceFindRequests | |||
@type {boolean} | |||
*/ | |||
coalesceFindRequests: boolean = false; | |||
|
|||
_coalesceFindRequests: boolean = false; |
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.
We don't want @private
documentation for this do we?
@snewcomer thank you for the fix! Can you please add a test that verifies this fixes the reported issue |
let post; | ||
module('init adapter coalesceFindRequests', function() { | ||
test('findMany - passes buildURL the requestType', function(assert) { | ||
this.owner.register('adapter:application', RESTAdapter.extend({ coalesceFindRequests: true })); |
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 think I might just move this to a unit test. This test file was sort of a pain to work with...
- We need to call unregister to register a new adapter with the adapter declaration shown above.
- Calling
store.adapterFor
in beforeEach makes it hard to unregister and register a new application adapter since the store_adapterCache
isn't cleared unless I force clear that private field - which doesn't seem like a good idea. The existence of this cache will result in the adapter registered in the beforeEach to be used. The approach I took here was to declare two module blocks to separate the concerns.
Still...a unit test is probably the way to go given the complexity here.
@@ -66,6 +66,8 @@ type SnapshotRecordArray = import('@ember-data/store/-private/system/snapshot-re | |||
@extends EmberObject | |||
*/ | |||
export default class Adapter extends EmberObject implements MinimumAdapterInterface { | |||
declare _coalesceFindRequests: boolean; |
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 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.
WAT. I missed this discussion and I am sad. tc39/proposal-class-fields#151 (comment)
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 think a better alternative here may be to do this within the constructor.
bfe50db
to
90b02d8
Compare
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.
After exploring this is the only implementation that works.
While we could use properties within the constructor for the base Adapter implementation, we would not be able to toggle the default from true
to false
for the RESTAdapter and JSONAPIAdapter which extend it for both .extends
and native class syntax.
A simpler implementation is only possible if we either drop the implementation in the base class entirely, change the base class to have the same default as the others, or stop extending from Adapter (we should probably actually do this, but can investigate it at another time).
This PR ensures that a user with non native class Adapter will be able to define their own
coalesceFindRequests
field.Will patch to release and beta.
close #7438
ref #7358