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

fix: Update typings for q1 2022 release #6051

Merged
merged 7 commits into from
Mar 31, 2022
Merged

Conversation

maribethb
Copy link
Contributor

@maribethb maribethb commented Mar 31, 2022

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

Proposed Changes

To generate this code, I started with Aaron's esmodules branch, which contains an esmodule-ified version of Blockly. Then, I changed the tsconfig in the file to have it output declaration files:

    // Tells TypeScript to read JS files, as
    // normally they are ignored as source files
    "allowJs": true,
    // Generate d.ts files
    "declaration": true,
    // This compiler run should
    // only output d.ts files
    "emitDeclarationOnly": true,
    // If you use `outDir` instead you get separate files which can be easier to debug specific issues
    // but harder to find the errors in the first place
    "outFile": "blockly.d.ts",

Then run npx tsc and fix errors until it worked. To resolve various errors (mostly cryptic messages like "Declaration emit for this file requires using private name 'IPathObject'. An explicit type annotation may unblock declaration emit.") I did the following (to the best of my memory, may have left out some changes):

  • Changed i_path_object.js constructor to not take any parameters. This made the aforementioned cryptic error disappear.
  • Realized a bunch of errors in the files were coming from not referencing the correct types. The esmodule code had converted all of the goog.require statements to import but not the goog.requireType statements. Aaron had previously made a script that converted these statements, which he kindly provided and then I modified to work for requireType. Aaron wrote this but I put it in this gist for longevity. This got rid of a bunch of errors about not being able to resolve the type, and got rid of a bunch of "any" types.
  • Got a similar "private name" error about using Metrics in WorkspaceSvg even though Metrics worked fine in other places like MetricsManager. The error persisted even if I removed the only mention of Metrics from WorkspaceSvg. Thanks again to Aaron, we resolved this by converting utils/metrics.js to a class.
  • Removed the types that I think came from the closure/base file.

At this point is approximately where I committed the changes so far to the "initial commit" because the tsc command succeeded with no errors, despite the output file still having problems. Additional changes made in separate commits:

  • The interfaces give errors, wherever MyClass implements SomeInterface appears we get an error like 'IASTNodeLocation' refers to a value, but is being used as a type here. Did you mean 'typeof IASTNodeLocation'? however its suggested solution of adding typeof doesn't work. If you go look at the definition of the interface mentioned, you'll see they are defined as functions instead of classes. So I changed them all to be defined as classes.
  • Remove "override" from a toString function, which ts doesn't like because the containing class is not a subclass of anything
  • Add an any type to some property because it's an error if there's an implicit any and I didn't want to guess the type it should be (though i'm pretty sure it should be (function(): function|string) | string)
  • remove spurious Array declaration (caused by the extra-state thing mentioned below)
  • fix the CopyData references which all had typeof even though CopyData was declared correctly as an exported member of the ICopyable namespace. this is something that could probably be fixed going forward with how we declare that property
  • fixed extraState by adding quotes to the typedef in source and regenerating. will fix this in source in another PR.

Test Coverage

none. sorry. but there are no "errors" in this file.

Additional Information

Please see caveats from last quarter as they still generally apply: #5802

  • interfaces are still missing some data; most of them have no properties or functions listed. but this is the same as last quarter so they haven't gotten worse.

@maribethb maribethb requested a review from a team as a code owner March 31, 2022 00:17
@maribethb maribethb requested a review from NeilFraser March 31, 2022 00:17
@maribethb maribethb requested review from rachel-fenichel and removed request for NeilFraser March 31, 2022 00:17
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.

LGTM. Thanks for the explanations of each of your steps, and for the gist (to you and Aaron). Hopefully this is the last quarter have to do this.

@maribethb
Copy link
Contributor Author

Fixed the extra-state problem and figured out the cause in the source code. this is an improvement from last quarter because it also fixed all the references to State in the other definitions instead of being any

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.

3 participants