feat(build): Support TypeScript in `core/` #6220

cpcallen · 2022-06-16T11:06:24Z

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

Closes #5892

Proposed Changes

Finish work to support TypeScript in core/ by making a small tweak to how the Closure Library is presented to Closure Compiler.
Convert core/utils/deprecation.js to TypeScript.
Enable clang-formatting of .ts files in core/ and blocks/.

Behaviour Before Change

Closure Compiler is fed files in closure/goog/ as input. If a .ts file imports closure/goog/goog.js, this causes the compiler to complain because the import will, after compilation by tsc, point to build/src/closure/goog, and CC won't let you name an input file goog.js unless it is in the same directory as base.js (or alternatively our base_minimal.js).

Behaviour After Change

All of closure/goog/*.js is passed through tsc and ends up in build/src/closure/goog/, so CC is reading goog.js and base_minimal.js from the same directory, making it happy.

Reason for Changes

See above.

Test Coverage

~~Passes npm test.~~ See comments below.
Playgrounds load without error and appear properly functional.

No changes to manual test procedures expected.

Additional Information

There are some issues with MigranTS output which means we cannot blindly copy it over the previous .js files—see individual commit messages for details.

The Closure Compiler complains if you try to feed it a file named goog.js which is not in the same directory as the Closure Library's base.js. Since tsc will "compile" goog.js when it encounters an "import ... from '.../goog.js'", it is necessary to also have tsc "compile" base.js and base_minimal.js, so they will come from the same directory. This necessitates some updates to paths in

This was done manually for test/proving purposes and might need to be corrected based on what MigranTS generated.

@Package

This manually applies certain changes from BeksOmega's ts/migration2 branch, but notably: - I did not apply the reordering of the doc comments at the top. - I applied the deletion of types and @Package from the JSDoc. - I preserved the import goog and goog.declareModuleId lines. - I have applied a whitespace change on line 37 which violates the styleguide; I want to figure out why clang-format is not fixing this.

And fix formatting issues introduced by MigranTS in deprecation.ts.

cpcallen · 2022-06-16T11:21:19Z

Hmm: good thing I accidentally made this a draft PR, because CI has caught a problem that I missed: it did pass npm test earlier, but commit 6484e42 removed the closure types from the JSDocs and thereby causes Closure Compiler to complain about call sites that omit the optional opt_use parameter:

gulp-google-closure-compiler: build/src/core/utils.js:72:4: ERROR - [JSC_WRONG_ARGUMENT_COUNT] Function module$exports$Blockly$utils.deprecation.warn: called with 3 argument(s). Function requires at least 4 argument(s) and no more than 4 argument(s).
  72|     deprecation.warn('Blockly.utils.noEvent', 'September 2021', 'September 2022');
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

build/src/core/utils.js:251:4: ERROR - [JSC_WRONG_ARGUMENT_COUNT] Function module$exports$Blockly$utils.deprecation.warn: called with 3 argument(s). Function requires at least 4 argument(s) and no more than 4 argument(s).
  251|     deprecation.warn('Blockly.utils.arrayRemove', 'December 2021', 'December 2022');
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

build/src/core/utils.js:320:4: ERROR - [JSC_WRONG_ARGUMENT_COUNT] Function module$exports$Blockly$utils.deprecation.warn: called with 3 argument(s). Function requires at least 4 argument(s) and no more than 4 argument(s).
  320|     deprecation.warn('Blockly.utils.runAfterPageLoad', 'December 2021', 'December 2022');
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

3 error(s), 0 warning(s), 94.7% typed

I think this is basically expected (:weeps in tsicle:) but we should probably think about exactly how we want to deal with it.
Maintaining Closure types in the JSDocs seems contrary to our reasons for moving to TypeScript, so the solution will be to adjust our Closure Compiler config—but do we just turn off the relevant diagnostics, or change our optimisation level, or what?

rachel-fenichel · 2022-06-16T14:59:33Z

core/utils/deprecation.ts

 * @alias Blockly.utils.deprecation.warn
- * @package


Replace package with internal rather than deleting it: https://api-extractor.com/pages/tsdoc/tag_internal/

Done.

Turns out this involves suppressing nonStandardJsDocs. But no problem.

Note that the @package annotation was deleted by the tooling that converted this file to TS, not by me. This will be a headache…

rachel-fenichel · 2022-06-16T16:08:12Z

Christopher and I discussed this morning and agreed to turn off error classes by adding them to JSCOMP_OFF as they turn up.

JSDoc and TSDoc are not the same. However, tsc can and will check types as we convert files to ts.

Trying to keep all of the comments compatible with both systems adds overhead and makes it take longer to get to where tsc can do the checks.

Given that we currently have a pile of ts waiting to merge, based on running migrants, and given that we are not releasing a full release at the end of the month, I've decided it's better to turn off these errors, merge ts files in, and then resolve errors raised by tsc.

I'm not sure why this didn't fail on my local machine previously; perhaps it succeeded only because of leftover files and would have failed if I'd run npm run clean.

Unfortunately TSC doesn't output type information in a form that Closure Compiler can understand, so the latter raises errors for situations like omitting an optional parameter. We may have to turn off more diagnostics in future, but for now this is sufficient.

Per comments on PR google#6220. This requires that we disable the nonStandardJsDocs diagnostic.

cpcallen · 2022-06-16T16:41:38Z

OK, looks like we're good at last.

I note that closure-make-deps apparently has its own type checking going on, and is beginning to get a bit whingy about things too:

[17:32:58] Starting 'buildDeps'...
WARNING in /Users/cpcallen/src/blockly/build/src/core/utils/deprecation.js at 20, 10: Missing type declaration.
WARNING in /Users/cpcallen/src/blockly/build/src/core/utils/deprecation.js at 21, 10: Missing type declaration.
WARNING in /Users/cpcallen/src/blockly/build/src/core/utils/deprecation.js at 23, 10: Missing type declaration.
WARNING in /Users/cpcallen/src/blockly/build/src/core/utils/deprecation.js at 25, 10: Missing type declaration.
WARNING in /Users/cpcallen/src/blockly/build/src/core/utils/deprecation.js at 27, 3: illegal use of unknown JSDoc tag "internal"; ignoring it. Place another character before the @ to stop JSCompiler from parsing it as an annotation.
WARNING in /Users/cpcallen/src/blockly/build/src/core/utils/deprecation.js at 20, 10: Missing type declaration.
WARNING in /Users/cpcallen/src/blockly/build/src/core/utils/deprecation.js at 21, 10: Missing type declaration.
WARNING in /Users/cpcallen/src/blockly/build/src/core/utils/deprecation.js at 23, 10: Missing type declaration.
WARNING in /Users/cpcallen/src/blockly/build/src/core/utils/deprecation.js at 25, 10: Missing type declaration.
WARNING in /Users/cpcallen/src/blockly/build/src/core/utils/deprecation.js at 27, 3: illegal use of unknown JSDoc tag "internal"; ignoring it. Place another character before the @ to stop JSCompiler from parsing it as an annotation.
WARNING in /Users/cpcallen/src/blockly/tests/mocha/test_helpers/common.js at 39, 3: Bounded generic semantics are currently still in development
WARNING in /Users/cpcallen/src/blockly/tests/mocha/test_helpers/common.js at 40, 3: Bounded generic semantics are currently still in development
WARNING in /Users/cpcallen/src/blockly/tests/mocha/test_helpers/common.js at 75, 3: Bounded generic semantics are currently still in development
WARNING in /Users/cpcallen/src/blockly/tests/mocha/test_helpers/common.js at 91, 3: Bounded generic semantics are currently still in development
WARNING in /Users/cpcallen/src/blockly/tests/mocha/test_helpers/common.js at 92, 3: Bounded generic semantics are currently still in development
[17:33:01] Finished 'buildDeps' after 3.15 s

(Previously it only complained about bounded generics.)

This tool appears to lack almost any control over error generation, which is annoying, but AFAIK these messages are harmless. I will see about suppressing—via grep -v if need be`—in a follow-on PR.

cpcallen · 2022-10-17T15:33:13Z

scripts/gulpfiles/build_tasks.js

  if (argv.compileTs) {
    chunks[0].entry = path.join(TSC_OUTPUT_DIR, chunks[0].entry);
  }


I should have deleted this in this PR. Fortunately noticed by an external contributor and corrected in #6431.

cpcallen added 5 commits June 16, 2022 09:02

docs(build): JSDoc update for JSCOMP_WARNING

5c3d72b

refactor(utils): Convert utils/deprecation.js to TypeScript

70516d5

This was done manually for test/proving purposes and might need to be corrected based on what MigranTS generated.

feat(build): clang-format .ts files

ca23daa

And fix formatting issues introduced by MigranTS in deprecation.ts.

cpcallen added component: build process PR: feature Adds a feature labels Jun 16, 2022

cpcallen mentioned this pull request Jun 16, 2022

Set up typescript compilation #5892

Closed

4 tasks

cpcallen added internal External contributions not accepted and removed internal External contributions not accepted labels Jun 16, 2022

cpcallen requested a review from rachel-fenichel June 16, 2022 11:21

cpcallen assigned rachel-fenichel Jun 16, 2022

BeksOmega linked an issue Jun 16, 2022 that may be closed by this pull request

Set up typescript compilation #5892

Closed

4 tasks

rachel-fenichel reviewed Jun 16, 2022

View reviewed changes

cpcallen added 3 commits June 16, 2022 17:22

fix(build): Fix sources for advanced compilation test

412e848

I'm not sure why this didn't fail on my local machine previously; perhaps it succeeded only because of leftover files and would have failed if I'd run npm run clean.

chore(utils): Use @internal where we previously used @Package

284103e

Per comments on PR google#6220. This requires that we disable the nonStandardJsDocs diagnostic.

cpcallen marked this pull request as ready for review June 16, 2022 16:37

cpcallen requested a review from a team as a code owner June 16, 2022 16:37

rachel-fenichel approved these changes Jun 16, 2022

View reviewed changes

rachel-fenichel merged commit 4070ffc into google:develop Jun 16, 2022

release-please bot mentioned this pull request Jun 16, 2022

chore(develop): release blockly 9.0.0 #6101

Closed

BeksOmega mentioned this pull request Jun 17, 2022

fix: tsc errors picked up from develop #6224

Merged

4 tasks

cpcallen deleted the ts branch June 23, 2022 15:05

cpcallen commented Oct 17, 2022

View reviewed changes

cpcallen mentioned this pull request Oct 27, 2022

fix(build): build/test on windows #6431

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(build): Support TypeScript in `core/` #6220

feat(build): Support TypeScript in `core/` #6220

cpcallen commented Jun 16, 2022 •

edited

Loading

cpcallen commented Jun 16, 2022

rachel-fenichel Jun 16, 2022

cpcallen Jun 16, 2022

rachel-fenichel commented Jun 16, 2022

cpcallen commented Jun 16, 2022

cpcallen Oct 17, 2022

feat(build): Support TypeScript in core/ #6220

feat(build): Support TypeScript in core/ #6220

Conversation

cpcallen commented Jun 16, 2022 • edited Loading

The basics

The details

Resolves

Proposed Changes

Behaviour Before Change

Behaviour After Change

Reason for Changes

Test Coverage

Additional Information

cpcallen commented Jun 16, 2022

rachel-fenichel Jun 16, 2022

Choose a reason for hiding this comment

cpcallen Jun 16, 2022

Choose a reason for hiding this comment

rachel-fenichel commented Jun 16, 2022

cpcallen commented Jun 16, 2022

cpcallen Oct 17, 2022

Choose a reason for hiding this comment

feat(build): Support TypeScript in `core/` #6220

feat(build): Support TypeScript in `core/` #6220

cpcallen commented Jun 16, 2022 •

edited

Loading