Skip to content
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

Test for #4948 (RxDocument.get() on additonalProperty) #4949

Closed
wants to merge 4 commits into from
Closed

Test for #4948 (RxDocument.get() on additonalProperty) #4949

wants to merge 4 commits into from

Conversation

adam-lynch
Copy link
Contributor

This PR contains:

  • IMPROVED TESTS

Describe the problem you have without this PR

This contains a test for #4948

@pubkey
Copy link
Owner

pubkey commented Sep 29, 2023

Hi @adam-lynch
I am not sure if this test is what you want it to be:
The test breaks because assert.strictEqual(myDocument.get('tags'), tags); throws.
But the tags object is never strictEqual to the thing you get from get('tags'), because RxDB will add getters for the population and observable fields.

Checking if the content is the same, works

 assert.deepStrictEqual(myDocument.get('tags').hello, tags.hello);
assert.deepStrictEqual(myDocument.get('tags').world, tags.world);

Also this is why toJSON().tags is strict equal, because toJSON() returns the plain data object, without any RxDB specific getters/setters.

@adam-lynch
Copy link
Contributor Author

adam-lynch commented Sep 29, 2023

@pubkey ah yes, I've improved the test now. While what you said was true, it's still failing now; both .get('tags') and .tags have unexpected extra fields (additionalProperties and type).

@pubkey
Copy link
Owner

pubkey commented Sep 29, 2023

Can you rebase so that the CI can run?

@pubkey
Copy link
Owner

pubkey commented Sep 29, 2023

RxDB uses a prebuild constructor (per collection) to create the RxDocument from the documentData that comes out of the storage.
This constructor knows all getters/setters, ORM methods and so on. This makes creating the RxDocument much faster compared to deep-cloning the data and adding these things to each document object.
Therefore it does not add getters for fields that are not known at collection-creation time (=fields that are not in the schema).

The only other options would be the proxy API, but the Proxy API is so slow that I do not want to use it for accessing document properties, because the performance is so important at this.

So atm toJSON()... is the only way to get your additional properties. Atm I am not sure how this could be fixed. But I will check the code and see what can be done.

@adam-lynch
Copy link
Contributor Author

OK I understand; if we want to use additionalProperties: true, we must access it via toJSON() / toMutableJSON().

@pubkey
Copy link
Owner

pubkey commented Sep 29, 2023

Yes, atm you need toJSON().
I see some options on how to fix that in the future, please leave this PR open.

@pubkey pubkey changed the title Test for #4948 Test for #4948 (RxDocument.get() on additonalProperty) Sep 29, 2023
pubkey added a commit that referenced this pull request Oct 4, 2023
@pubkey
Copy link
Owner

pubkey commented Oct 4, 2023

I did some performance tests and refactorings. This will be fixed in the next major version. #5063

pubkey added a commit that referenced this pull request Oct 5, 2023
* IMPROVE property access performance

* FIX #4949 RxDocument.get() on additonalProperty

* IMPROVE performance

* IMPROVE performance

* FIX bun
@pubkey pubkey closed this in 46f3636 Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants