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

Review contents of blockly.js #5208

Closed
28 of 33 tasks
Tracked by #5026
rachel-fenichel opened this issue Jul 24, 2021 · 2 comments
Closed
28 of 33 tasks
Tracked by #5026

Review contents of blockly.js #5208

rachel-fenichel opened this issue Jul 24, 2021 · 2 comments

Comments

@rachel-fenichel
Copy link
Collaborator

rachel-fenichel commented Jul 24, 2021

blockly.js is full of public stuff that doesn't really fit on any namespace. This is a problem because internal code requiring it causes circular dependencies.

The fix is to move everything in blockly.js out into separate files and require those from inside the codebase, while leaving aliases behind in blockly.js for anything that should stay publicly accessible.

In some cases, we also need setters so that external code can change those values.

In other cases, properties should just not be exported.

This issue is for tracking decisions about these properties and whether those decisions have been implemented.

Needs decision:

  • hideChaff
  • alert
  • confirm
  • prompt
  • hueToHex

Can be internal:

Exported function, but moved to a different file and aliased.

Exported function, used only externally and therefore left in blockly.js:

  • isNumber: Used in generators
  • defineBlocksWithJsonArray: Used in block definitions
  • svgSize: Deprecated already, and marked for removal in March 2022
  • resizeSvgContents

Exported read-only property:

  • [ ]

Exported settable property:

Can be removed:

@rachel-fenichel rachel-fenichel added issue: feature request Describes a new feature and why it should be added issue: triage Issues awaiting triage by a Blockly team member type: cleanup and removed issue: triage Issues awaiting triage by a Blockly team member issue: feature request Describes a new feature and why it should be added labels Jul 24, 2021
@rachel-fenichel rachel-fenichel added this to the 2021_q3_release milestone Jul 24, 2021
@rachel-fenichel
Copy link
Collaborator Author

@cpcallen This is obviously incomplete, but is a place to start in on pulling apart blockly.js. I think this is going to take multiple passes. For each thing we need to decide what category it belongs in and where to move it to, possibly including creating new files.

@rachel-fenichel
Copy link
Collaborator Author

rachel-fenichel commented Sep 10, 2021

Breaking out parts of the remaining work:

  • Move alert, confirm, and prompt to a new file
    • Move functions
    • Update all internal uses
    • Add getters and setters in blockly.js for backwards compatibility
    • Record rename in renames file
  • Move hideChaff to a new file
    • Move function and resolve any circular dependencies
    • Update all internal references
    • Record rename in renames file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant