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: Rename Mutator#workspace back to workspace_ for compatibility with library blocks #6634

Merged
merged 2 commits into from
Nov 18, 2022

Conversation

rachel-fenichel
Copy link
Collaborator

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

Fixes #6630

Proposed Changes

Revert the renaming of the mutator workspace: workspace -> workspace_ and make it public.

Behavior Before Change

Workspace was not officially public but was being used by procedure mutators, and it broke when I renamed it.

Behavior After Change

Existing procedure mutators work now.

Reason for Changes

Revert a rename that actually broke something.

Test Coverage

  1. Open the playground
  2. Pull out a procedure definition block
  3. Open the mutator
  4. Add two parameters
  5. Rename one of the parameters

Beka reported that this triggered the issue, and it now works.

Beka is also adding tests for procedures as part of the rest of her work, which should help avoid this in future.

Documentation

None

Additional Information

The built-in blocks blur the line between internal and public. My position: any property the blocks access (e.g. in mutators) is public and changing it requires a true deprecation. Blocks shouldn't be accessing private properties, and honestly should just be using getters where possible, but for a long time we encouraged developers to copy our block definitions and modify them as they saw fit, so there are probably blocks out there that access the same properties.

This needs to be cherry-picked into develop and released as a patch.

@rachel-fenichel rachel-fenichel requested a review from a team as a code owner November 17, 2022 01:40
@github-actions github-actions bot added the PR: fix Fixes a bug label Nov 17, 2022
@BeksOmega
Copy link
Collaborator

I would prefer to revert the original change, but mark workspace_ as @internal instead of purely public. I don't want /more/ people to start using this, when really it shouldn't be public at all, and people should use getWorkspace().

@rachel-fenichel
Copy link
Collaborator Author

I'm okay with switching it to reverting the original change, but I want workspace_ to be public. I'm treating internal as fair game for renames, so it's not accurate to make it internal and could lead to the same issue in the future. We can probably put deprecated in there so that typescript will warn, if that helps.

@BeksOmega
Copy link
Collaborator

I'm okay with switching it to reverting the original change, but I want workspace_ to be public. I'm treating internal as fair game for renames, so it's not accurate to make it internal and could lead to the same issue in the future. We can probably put deprecated in there so that typescript will warn, if that helps.

Deprecated is a reasonable compromise!

@rachel-fenichel
Copy link
Collaborator Author

After investigation:

  • I can't mark it deprecated because then I get warnings and compiler errors about even internal use`
  • Making it formally public isn't great because it formally extends the API and that's not something I should be doing in a reversion/cherry-pick.
  • Leaving it formally private isn't super safe because it could get renamed again, either manually or by renames during compilation.
  • I could maybe put it in the renaming file to hopefully make the update happen even in block definitions.

I have put it back to private workspace_ because that's the most direct reversion of the actual breaking change.

@BeksOmega ptal

@rachel-fenichel rachel-fenichel merged commit a1eb423 into google:develop Nov 18, 2022
@cpcallen cpcallen changed the title fix: make mutator workspace public fix: Rename Mutator#workspace back to workspace_ for compatibility with library blocks Nov 21, 2022
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Nov 21, 2022
maribethb pushed a commit to maribethb/blockly that referenced this pull request Nov 21, 2022
* fix: make mutator workspace public

* fix: make workspace private again
@maribethb maribethb mentioned this pull request Nov 21, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block definitions access some renamed private properties
2 participants