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.

Sign up for GitHub

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

Jump to bottom

refactor: Rework document rendering to avoid data duplication and mutation #68

Merged
merged 2 commits into from
Dec 2, 2021
Merged

refactor: Rework document rendering to avoid data duplication and mutation #68

merged 2 commits into from
Dec 2, 2021

Conversation

AndrewSisley
Copy link
Contributor

Closes #34

@AndrewSisley
Use dedicated property for collection name
6593b77 
Instead of overwriting the field name
@AndrewSisley AndrewSisley added area/query Related to the query component refactor This issue specific to or requires *notable* refactoring of existing codebases and components labels Dec 1, 2021
@AndrewSisley AndrewSisley self-assigned this Dec 1, 2021
@AndrewSisley AndrewSisley linked an issue Dec 1, 2021 that may be closed by this pull request
Refactor renderDoc and renderNode to avoid duplicate instances for same doc type #34
Closed
jsimnz
jsimnz requested changes Dec 1, 2021
View reviewed changes
Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

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

hey @AndrewSisley

big fan of this approach, very clean!

I only ask that you document the renderInfo.render(...) a little bit, so readers can get a (brief) high level description/overview without going through the entire func. Or give context (eg: that it uses the recursive structure of the gql query).

Also like that you rely on the Selection interface here to support generic objs.

@AndrewSisley AndrewSisley force-pushed the sisley/refactor/I34-render-deduplication branch from 983158d to fead82b Compare December 1, 2021 23:57
@AndrewSisley AndrewSisley requested a review from jsimnz December 1, 2021 23:57
@AndrewSisley
Copy link
Contributor Author

hey @AndrewSisley

big fan of this approach, very clean!

I only ask that you document the renderInfo.render(...) a little bit, so readers can get a (brief) high level description/overview without going through the entire func. Or give context (eg: that it uses the recursive structure of the gql query).

Also like that you rely on the Selection interface here to support generic objs.

Thanks! And thanks for asking for more docs - that function was quite dense even without considering it's recursive nature... Should be updated now

jsimnz
jsimnz approved these changes Dec 2, 2021
View reviewed changes
Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

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

Awesome!

@AndrewSisley AndrewSisley merged commit d791c93 into develop Dec 2, 2021
@AndrewSisley AndrewSisley deleted the sisley/refactor/I34-render-deduplication branch December 2, 2021 00:16
@jsimnz jsimnz mentioned this pull request Dec 2, 2021
Closed
@orpheuslummis orpheuslummis mentioned this pull request Jan 29, 2022
Closed
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
@AndrewSisley
refactor: Rework document rendering to avoid data duplication and mut…
44e0a6a 
…ation (sourcenetwork#68)

* Use dedicated property for collection name

Instead of overwriting the field name

* Render without mutating source doc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsimnz jsimnz jsimnz approved these changes

@shahzadlone shahzadlone Awaiting requested review from shahzadlone

Assignees

@AndrewSisley AndrewSisley

Labels
area/query Related to the query component refactor This issue specific to or requires *notable* refactoring of existing codebases and components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor renderDoc and renderNode to avoid duplicate instances for same doc type
2 participants
@AndrewSisley @jsimnz