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

Preparation for goog.module transition: base.js, deps.js #5019

Merged
merged 7 commits into from
Jul 13, 2021

Conversation

cpcallen
Copy link
Contributor

@cpcallen cpcallen commented Jul 12, 2021

The basics

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

The details

Proposed Changes

  • Use the current standard Closure Library version of closure/goog/base.js.
  • In uncompiled mode, load dependency graph from tests/deps.js.
    • New gulp task buildDeps (npm run build:deps) to create tests/deps.js.
    • blockly_uncompressed.js is now handwritten.
      • And simplified; in particular, BLOCKLY_BOOT is gone.
      • And linted!
    • Old gulp task buildUncompressed is deleted.
  • For consistency with the Closure documentation and base.js, consistently refer to what blockly_uncompressed.js is used for as "uncompiled mode" rather than "uncompressed mode" (but don't yet rename it to blockly_uncompiled.js).
  • Have eslint ignore "undefined" global variable exports, used to specify exports from goog.module modules.

Reason for Changes

The version of base.js we have been using is a cut-down copy of an older version, stripped of everything that wasn't strictly needed for Blockly. We now need things that were previously stripped out, so restore the current version from https://github.com/google/closure-library/blob/master/closure/goog/base.js. We can always strip out unused sections again in future - once we know which sections those are.

Loading dependency graph info from tests/deps.js (instead of having it embedded in blockly_uncompressed.js helps maintain a clear separation between generated and handwritten code, avoids having code duplicated between the generator script and its output, and helped find some actual errors the handwritten parts because they can now be linted. It also more closely follows the Closure Library convention.

Additional Information

This PR (or another to the same effect) must be merged to the goog_module branch before any others that actually carrying out goog.provides -> goog.module migration.

The version of base.js we have been using is a cut-down copy of an older
version, stripped of everything that wasn't strictly needed for Blockly.

We now need things that were previously stripped out, so restore the
current version from
https://github.com/google/closure-library/blob/master/closure/goog/base.js
as of commit 5cb5e81 (2021-06-04).

We can always strip out unused sections again in future - once we know
which sections those are.
@cpcallen cpcallen requested a review from a team as a code owner July 12, 2021 00:48
* New gulp task buildDeps (npm run build:deps) to create tests/deps.js.
* Old gulp task buildUncompressed is deleted.
* blockly_uncompressed.js is now handwritten.
  * And simplified; in particular, BLOCKLY_BOOT is gone.
  * And linted!
* For consistency with the Closure documentation and base.js,
  consistently refer to what blockly_uncompressed.js is used for as
  "uncompiled mode" rather than "uncompressed mode" (but don't yet
  rename it to blockly_uncompiled.js)
cpcallen added 3 commits July 12, 2021 02:54
Modules defined using goog.module declare their exported functions
by assigning them to properties on an object which is the value of
the variable "exports" (e.g., "exports.MyClass = class { ... };").

Normally eslint would complain about this variable being undefined,
but this commit suppresses these errors.
Some type annotations were missing curly brackets, which makes
closure-make-deps emit uninteresting warnings.  Now any output
from the command will be informative and related to whatever
one is presently working on.
eslint appears to be confused by the leading JSDoc comment on the
continuation line and suggests it should be indented only 4 rather
than 8 spaces.
Copy link
Collaborator

@rachel-fenichel rachel-fenichel left a comment

Choose a reason for hiding this comment

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

Approved after lint fix.

For whatever reason eslint was rejecting the (styleguide-correct)
indentation of the line beginning with "/** @type ...", and in my
naïvete I got a little more zealous than I intended in suppressing
the error.
@cpcallen
Copy link
Contributor Author

cpcallen commented Jul 13, 2021

Blocked on #5018.

This resolves a conflict in `blockly_uncompressed.js`, and missing
updates to `test/deps.js`, caused by PR google#5041.
@cpcallen cpcallen merged commit 8b81043 into google:goog_module Jul 13, 2021
@cpcallen cpcallen deleted the goog.module-prep branch July 13, 2021 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants