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

[1.x] Fix resolving shared dot props #636

Closed

Conversation

lepikhinb
Copy link
Contributor

@lepikhinb lepikhinb commented Jun 1, 2024

Based on #634

I think #620 introduced a breaking change when sharing props through the middleware, as described in #633

This PR includes extra tests to ensure shared props can be included and excluded from partial responses.

@lepikhinb lepikhinb marked this pull request as ready for review June 1, 2024 07:09
@VicGUTT
Copy link

VicGUTT commented Jun 1, 2024

Same comment as in #634 (comment).

Might be worth investigation if the PR #631 also solve this issue?

@lepikhinb
Copy link
Contributor Author

@VicGUTT it doesn't. However, I think #631 can be simplified a bit.

@VicGUTT
Copy link

VicGUTT commented Jun 2, 2024

Alright, too bad. Thanks for confirming.

Sure, in what way? That PR simply reverted back the method "resolvePropertyInstances" to what it previously was

@lepikhinb
Copy link
Contributor Author

@reinink I rewrote the entire partial request resolving logic. It seems that the only way to support nested partial props without introducing any breaking changes is to check each prop independently to determine whether it should be included or excluded from the partial response.

Additionally, we need to resolve dot-notated props last to ensure they are merged after any parent closures are resolved.

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.

3 participants