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

refactor!(blocks): Rename module Blockly.blocks.all to Blockly.blocks; former Blockly.blocks back to Blockly.Blocks #5946

Closed
wants to merge 2 commits into from

Conversation

cpcallen
Copy link
Contributor

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide

The details

Proposed Changes

Behaviour Before Change

  • Code requiring the blocks dictionary object does const {Blocks} = goog.require('Blockly.blocks');,
    • or uses a script tag to load blockly_compressed.js and then access the dictionary via Blockly.Blocks.
  • Code loading library blocks does goog.require('Blockly.blocks.all');,
    • or uses a <script> tag to load blocks_compressed.js and then accesses exports via Blockly.blocks.all.

Behaviour After Change

  • Code requiring the blocks dictionary object does const {Blocks} = goog.require('Blockly.Blocks');
    • or uses a script tag to load blockly_compressed.js and then access the dictionary via Blockly.Blocks (unchanged).
  • Code loading library blocks does goog.require('Blockly.blocks');,
    • or uses a <script> tag to load blocks_compressed.js and then accesses exports via Blockly.blocks.

There will be no changes visible when the blocks chunk is loaded via ES module import or CJS require.

Reason for Changes

This change was originally created as an alternative fix for issue #5932. PR #5945 is a better fix for the actual underlying cause of that issue, but I think it is worth considering doing these renamings anyway simply because of providing a more sensible path for the blocks modules exports object for those loading the compiled chunks directly.

Test Coverage

Tested on:

  • Desktop Chrome
  • npm test

Additional Information

Having thecore/blocks.js module named Blockly.Blocks is less in accord with the styleguide, but more consistent with the (mis)capitalisation of its main export, the .Blocks dictionary object.

The need for naming of modules will shortly go away entirely when we convert from goog.module to ES modules.

This is a partial revert of PR google#5515, to allow Blockly.blocks.all
to be renamed Blockly.blocks.
This is a breaking change (only) because the exports object from the
`blocks_compressed.js` chunk will be accessed as `Blockly.blocks`
instead of `Blockly.blocks.all` when the chunk is loaded in a
browser via a `<script>` tag.  There will be no changes visible
when the chunk is loaded via ES module `import` or CJS `require`.
@cpcallen cpcallen added component: build process component: library blocks breaking change Used to mark a PR or issue that changes our public APIs. PR: chore General chores (dependencies, typos, etc) labels Feb 15, 2022
@cpcallen cpcallen requested a review from a team as a code owner February 15, 2022 14:45
@cpcallen cpcallen requested a review from alschmiedt February 15, 2022 14:45
@cpcallen
Copy link
Contributor Author

We could also consider renaming all of the blocks/ modules to Blockly.libraryBlocks.*, or to BlocklyBlocks.* or even just blocks.*, but in chat this option seemed to be slightly preferred.

@rachel-fenichel
Copy link
Collaborator

After discussion with Chris:

  • Rename to Blockly.libraryBlocks to make the module name make more sense when referred to directly.
  • Find the right place to put documentation about how to access this through requires, through script tags, and through modules, since the correct code varies by context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Used to mark a PR or issue that changes our public APIs. component: build process component: library blocks PR: chore General chores (dependencies, typos, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants