Skip to content

Delete src/additional-typings.d.ts#3325

Merged
robertbrignull merged 1 commit intomainfrom
robertbrignull/delete-additional-typings
Feb 7, 2024
Merged

Delete src/additional-typings.d.ts#3325
robertbrignull merged 1 commit intomainfrom
robertbrignull/delete-additional-typings

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

I'm looking at this file because I'm trying to remove uses of any. In this case it's not clear whether to change it to unknown or delete the entire file.

The comment in this file says it is needed to make compilation succeed, but if I delete it then compilation still works. Therefore hopefully that means this file is no longer needed. It was introduced back in #1111 and hasn't been touched since, and we've changed the build system quite a bit since then.

I've tried using the graph viewer to see if it still works and unfortunately I can't get it to work on main or this branch. In both cases I get a blank screen 🤷🏼

Screenshot 2024-02-07 at 13 01 04

@aeisenberg, @hvitved, @nickrolfe, I believe you were the originators of the graph viewer. What do you think?

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@robertbrignull robertbrignull requested a review from a team February 7, 2024 13:46
@robertbrignull robertbrignull requested a review from a team as a code owner February 7, 2024 13:47
@shati-patel
Copy link
Copy Markdown
Contributor

shati-patel commented Feb 7, 2024

I've tried using the graph viewer to see if it still works and unfortunately I can't get it to work on main or this branch. In both cases I get a blank screen 🤷🏼

I can confirm it works both on main and on this branch for me 🎉

image

It looks like your graph is trying to use the wrong query 🤔 (printAst instead of printCfg)?

@aeisenberg
Copy link
Copy Markdown
Contributor

I think this is safe since this is only about making tsc happy. I'm not sure why it is working now, but wasn't before. Possibilities:

  1. Have we updated d3 to a newer version so the typings are more flexible?
  2. Have we changed how we compile things so that the missing typings are no longer an error?

Either way, if it's compiling, then it's safe to remove.

@robertbrignull
Copy link
Copy Markdown
Contributor Author

Thanks both for investigating and confirming. I agree if tsc is happy then it should be fine. I just wanted to check.

@robertbrignull robertbrignull merged commit 5fe5f70 into main Feb 7, 2024
@robertbrignull robertbrignull deleted the robertbrignull/delete-additional-typings branch February 7, 2024 17:14
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