Skip to content

[fix]: add render context validation check to makeSink #239

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

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

jamieQ
Copy link
Contributor

@jamieQ jamieQ commented Aug 15, 2023

Issue

the makeSink(...) method of the concrete RenderContext implementation did not include the same validation assertion that all other methods of the type have. this could lead to issues where a context was retained outside of a render(...) method and if it was used to make a sink outside that method, we would not detect that unexpected state.

Description

  • adds a assertStillValid check to the makeSink method
  • adds doc comments for the method

note: personally i think we should aim to go a bit further and make the validation assertion a precondition as well as destroy the reference to the internal context when the externally-accessible context is invalidated. but that sort of change likely requires gathering some data about places that might currently break if we were to fail more 'aggressively' in that scenario.

Checklist

  • Unit Tests
  • UI Tests
  • Snapshot Tests (iOS only)
  • I have made corresponding changes to the documentation

@jamieQ jamieQ requested a review from a team as a code owner August 15, 2023 12:13
@jamieQ jamieQ merged commit 9e40cde into main Aug 15, 2023
@jamieQ jamieQ deleted the jquadri/validate-make-sink branch August 15, 2023 14:29
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