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

fix: Fix blocks with mutators. #6440

Merged
merged 2 commits into from
Sep 27, 2022
Merged

fix: Fix blocks with mutators. #6440

merged 2 commits into from
Sep 27, 2022

Conversation

gonfunko
Copy link
Contributor

@gonfunko gonfunko commented Sep 20, 2022

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

#6420

Proposed Changes

Passes the block to the Mutator constructor.

Behavior Before Change

Mutators would fail to initialize properly.

Behavior After Change

Mutators initialize and work as expected.

Reason for Changes

As part of the migration to TS, the Mutator constructor was changed to require the block it's attached to. Most existing block definitions had not been updated to pass this in. This PR updates those definitions to invoke the new constructor correctly.

Alternatively, I could try to revert the previous change to the Mutator constructor and pass null as the block value to the Icon superclass constructor as it did in v8, which probably requires some other changes to types.

Information for Deprecating Change

This PR adds a deprecation warning if you call a mutator or Icon without passing the block the mutator/icon is attached to. In the future, you will be required to pass the block. If you are affected, then find the offending mutator and include the attached block as the last argument to the mutator constructor.

Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

LGTM on its own merits, but (as discussed in standup today) please hold off on merging this until a decision is made about whether we want to revert the change to the signature of the Mutator constructor.

@gonfunko
Copy link
Contributor Author

Per team discussion, I reverted the changes to the constructor, but added deprecation warnings for using them without passing a block.

@gonfunko gonfunko added the deprecation This PR deprecates an API. label Sep 27, 2022
@BeksOmega BeksOmega self-requested a review September 27, 2022 20:16
@BeksOmega BeksOmega assigned BeksOmega and unassigned cpcallen Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This PR deprecates an API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants