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

Makes EditorView non-nullable in useLayoutGroupEffect and useEditorEventCallback #83

Merged
merged 5 commits into from
Jan 30, 2024

Conversation

coolov
Copy link
Contributor

@coolov coolov commented Jan 25, 2024

What?

Currently the view prop in useLayoutGroupEffect and useEditorEventCallback is nullable, this means that we need to assert that the value exists before using it:

const onClick = useEditorEventCallback((view) => {
  if (view) {
    // do things
  }
});

By making the value non-nullable we no longer need this check!

How?

This PR proposes that we only fire the callbacks in the effects if view is set. This way we can guarantee that the value is never null and always of type EditorView.

So why is the value sometimes null in the first place? This has to do with how the initial render bootstraps the EditorContext that holds the EditorView. See @smoores-dev comment in this thread:

On the first render pass, the view will be null, because it's constructed in a layout effect. That means that we sort of have to type the context as returning an optional view, even though in practice, I think it would always be defined by the time the callback actually runs.

For more details about how the creation of the EditorContext is deferred, see useEditorView

@coolov coolov requested review from smoores-dev and a team as code owners January 25, 2024 22:42
@smoores-dev
Copy link
Collaborator

Awesome, thanks @coolov. I actually think this is a pretty big win; more than one user has expressed confusion or annoyance that the view might be null.

This feels good to me, though as @tilgovi pointed out in the other thread, undoing this would be a breaking change, so it's worth thinking about critically. @tilgovi, did you have any other thoughts you wanted to raise on the topic?

@tilgovi tilgovi merged commit 0662f84 into main Jan 30, 2024
2 checks passed
@tilgovi tilgovi deleted the only-run-editor-event-callback-if-view-is-availible branch January 30, 2024 16:07
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