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: updates accessors for getMainWorkspace and getSelected #6313

Merged
merged 5 commits into from
Aug 4, 2022

Conversation

BeksOmega
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

Work on #5857

Proposed Changes

  • Adds an accessor for getSelected to Blockly
  • Updates the setters and getters in main.js to point you to the accessors on Blockly.
  • Updates some of the docs.

Behavior Before Change

No accessor

Behavior After Change

Accessor!

Reason for Changes

We don't really want external developers to have to use common, since it was just used to break some circular dependencies.

Test Coverage

N/A

Documentation

We probably have documentation that still points to mainWorkspace and selected that should be updated.

Additional Information

Removal date is September 2022 so that we can remove main.js as soon as possible.

@BeksOmega BeksOmega requested a review from a team as a code owner August 4, 2022 21:49
@BeksOmega BeksOmega requested a review from gonfunko August 4, 2022 21:49
core/blockly.ts Outdated
@@ -315,14 +315,19 @@ export function hideChaff(opt_onlyClosePopups?: boolean) {
* Returns the main workspace. Returns the last used main workspace (based on
* focus). Try not to use this function, particularly if there are multiple
* Blockly instances on a page.
* @return The main workspace.
* @see Blockly.common.getMainWorkspace
* @alias Blockly.getMainWorkspace
*/
// AnyDuringMigration because: Property 'getMainWorkspace' does not exist on
// type 'void'.
export const getMainWorkspace = (common as AnyDuringMigration).getMainWorkspace;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this cast actually necessary or can it be removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed all instances of AnyDuringMigration from blockly.ts except for the one in populateProcedures since I want to get rid of that as part of the shared procedures project anyway, and it's not external facing.

@@ -85,9 +85,15 @@ Object.defineProperties(Blockly, {
*/
mainWorkspace: {
set: function(x) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and for selected, the deprecation notice for set*() doesn't really make sense since it points you to the getter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True. The other option is we just don't include a suggested replacement in the set-case (since I don't want people to set the main workspace or selected object themselves). I thought it would be better to include the get method so that people have a little bit of info, but if it's confusing I'm more than happy to remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, I think you're right that the context is useful. Leaving as-is SGTM.

@BeksOmega BeksOmega merged commit fd127f6 into google:develop Aug 4, 2022
@BeksOmega BeksOmega deleted the feat/accessors branch October 4, 2022 18:08
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.

2 participants