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: Remove uses of AnyDuringMigration from scrollbar.ts, workspace_svg.ts, contextmenu_registry.ts, and component_manager.ts #6318

Merged
merged 7 commits into from
Aug 22, 2022

Conversation

gonfunko
Copy link
Contributor

@gonfunko gonfunko commented Aug 5, 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

Resolves

Part of #5857

Proposed Changes

Removes uses of AnyDuringMigration.

@gonfunko gonfunko requested a review from a team as a code owner August 5, 2022 21:47
@gonfunko gonfunko requested a review from cpcallen August 5, 2022 21:47
Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

Approved subject to comments below, to be rectified as you deem appropriate.

Comment on lines 197 to 198
componentDataList.forEach(function(ComponentDatum) {
components.push(ComponentDatum.component);
components.push(ComponentDatum.component as T);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to specify the type of the argument named ComponentDatum? (And maybe give it a less confusing name, since that seems to be the name of a type as well.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TS infers the type as ComponentDatum from the typing of componentDataList. If you're referring to the "as T" cast I think we're stuck with that because, while capabilityToComponentIds_ should be giving us only ComponentDatums whose component field is of type T, TS doesn't know that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I don't fully understand the purpose of the ComponentDatum type here, but the code as revised in this PR looks plausible enough so don't let that stop you submitting it!

@@ -1212,7 +1141,7 @@ export class WorkspaceSvg extends Workspace implements IASTNodeLocationSvg {
*/
translate(x: number, y: number) {
if (this.useWorkspaceDragSurface_ && this.isDragSurfaceActive_) {
this.workspaceDragSurface_.translateSurface(x, y);
this.workspaceDragSurface_?.translateSurface(x, y);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why ?. here instead of !.? Was the old code wrong in assuming that .workspaceDragSurface_ can't be undefined here?

@cpcallen cpcallen added component: TypeScript PR: chore General chores (dependencies, typos, etc) labels Aug 20, 2022
@gonfunko gonfunko merged commit 9454b70 into google:develop Aug 22, 2022
@gonfunko gonfunko deleted the anyduringmigration branch August 22, 2022 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: TypeScript PR: chore General chores (dependencies, typos, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants