-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix: fix wrong accumulator return type for initializeModel function #396
fix: fix wrong accumulator return type for initializeModel function #396
Conversation
🦋 Changeset detectedLatest commit: 156b6d0 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
eec25ec
to
699aedc
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.
Looks good overall. One nit
describe('when related data model has a composite key', ()=>{ | ||
test('belongsTo: related data is queried with foreign keys', async() => { |
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.
describe blocks are helpful when grouping tests together, especially when we need to do some setup or teardown related to some shared grouping/testcase relationship.
In cases like this, I might just use a test without the wrapper.
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.
That makes sense, I have removed the describe() wrapper.
|
||
type CompositeKeySchema = ClientSchema<typeof compositeKeySchema>; | ||
|
||
test('belongsTo: related data is queried with foreign keys', async() => { |
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.
Just want to confirm: This test fails in the absence of the change?
Related issues:
Description of changes:
This change adds a defined-behavior integration test for initializeModel function. The test fails since the function has a wrong return type when it accumulates sort keys from data.
The change then fixes the return type and passes the test.
Example of why we need this:
If customers have two data models
Order
andCustomer
, whereOrder
has reference fields that matchCustomer
's identifier, and abelongsTo
relationship field withCustomer
:when they try to lazy load the customer data from an order:
they will get the following error:
Uncaught (in promise) TypeError
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.