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

[R] fix (x-bundle): add deepsync to the inseret/update executors #281

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

y-moufakkr
Copy link
Collaborator

close #260

@Fen747
Copy link
Collaborator

Fen747 commented Apr 24, 2022

I don’t understand how you changes are allowing the executor to deepsync the inpu. All it does is Fetch the inserted / updated doc and deepsync it, which will have no effect (unless you inserting fields in the root document, which is undesirable)

@y-moufakkr y-moufakkr changed the title [R] fix (x-bundle): add deepsync to the inseret/update executors [WIP] fix (x-bundle): add deepsync to the inseret/update executors Apr 24, 2022
@y-moufakkr y-moufakkr force-pushed the feat/x-bundle/260-deepsync-in-executors branch from d70b1b8 to a5ccae5 Compare April 24, 2022 18:34
@y-moufakkr y-moufakkr changed the title [WIP] fix (x-bundle): add deepsync to the inseret/update executors [R] fix (x-bundle): add deepsync to the inseret/update executors Apr 24, 2022
@@ -155,6 +156,7 @@ export function CheckDocumentExists<T>(
*/
export function ToDocumentInsert<T>(
collectionClass: Constructor<Collection<T>>,
options?: InsertUpdateExecutorOptions,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduces breaking changes. Let's use either field, or { field: "xxx", deepSync: "xxx" })

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extend() I don't think it's ever used. But add it to those options, so it's a string (field) or an object of options.

Copy link
Contributor

@theodorDiaconu theodorDiaconu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check comments

@theodorDiaconu theodorDiaconu changed the title [R] fix (x-bundle): add deepsync to the inseret/update executors [WIP] fix (x-bundle): add deepsync to the inseret/update executors Apr 27, 2022
@y-moufakkr y-moufakkr changed the title [WIP] fix (x-bundle): add deepsync to the inseret/update executors [R] fix (x-bundle): add deepsync to the inseret/update executors Apr 28, 2022
if (typeof options.deepSync !== "boolean") {
const deepSyncFields = {};
(Array.isArray(options.deepSync)
? options.deepSync
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we are specifying which fields we're going to deep sync? How does this help us, since document[key] already exists and then you merge it below, won't it be like that everytime?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now only allow deepSync as bool.

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.

Enable deepsync in executors or create a new deepsync executor
3 participants