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

feat: Allow each extension to add a change handler. #7489

Closed
wants to merge 1 commit into from

Conversation

laurensvalk
Copy link
Contributor

The basics

This is a draft PR for discussion; see notes below. Also still needs tests.

The details

Resolves

I was having some difficulty "merging" several onchange handlers, i.e. adding one to a block that already uses one as part of another extension.

Originally posted by @laurensvalk in #7483 (comment)

Yeah there's not a good way to do this at the moment :/ we have similar problems with other mixins.

Originally posted by @BeksOmega in #7483 (comment)

Proposed Changes (and proposed next step)

Implement Block.addOnChange. Similar to Block.setOnChange but adds them to a list, and later calls them in the same order they were added.

Block.setOnChange is kept to preserve backwards compatibility, though some duplicate code could be merged. The handler set by setOnChange could always be called first (even if it was set later) for backwards compatibility.

In practice, this means users can easily enforce the order when they fill the extensions[] list on the block specification.

Reason for Changes

When a block already has an onChange handler, calling setOnChange from another extension currently overrides it.

This makes it difficult to keep extensions small and local, i.e. an extension that deals with a field's final value as discussed in #7483, combined with some other extension on the block.

Similarly, it means onChange handlers can't easily be used with blocks that already use them internally, like those in block-shareable-procedures.

Test Coverage

TBD

Documentation

TBD

When a block already has an onChange handler, calling setOnChange from another extension currently overrides it.
@laurensvalk laurensvalk requested a review from a team as a code owner September 14, 2023 12:11
@github-actions github-actions bot added the PR: feature Adds a feature label Sep 14, 2023
@BeksOmega
Copy link
Collaborator

Hello! Before reviewing this, just want to check if using a non-mixin extension with a call to this.workspace.addChangeListener works for your use-case?

@laurensvalk
Copy link
Contributor Author

It seemed useful more generally since you mentioned running into the same problem, but I'm happy to try it another way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature Adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants