Skip to content

Introduce VsCodeWindow to provide types for the fields we're reading/writing#3327

Merged
robertbrignull merged 1 commit intomainfrom
robertbrignull/VsCodeWindow
Feb 8, 2024
Merged

Introduce VsCodeWindow to provide types for the fields we're reading/writing#3327
robertbrignull merged 1 commit intomainfrom
robertbrignull/VsCodeWindow

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

There are a few places where we are reading/writing extra fields to the window object. I believe these fields which are provided by VS Code, but sometimes in the tests we are mocking them ourselves.

Instead of using window as any we can define the type that we expect it to take using declare. This is telling the typescript compiler that a variable exists with this given name and type and it will just believe us.

Since this is just telling the compiler what type things are, it shouldn't change anything at runtime. Therefore hopefully if this compiles then it's safe 🤞🏼

We already do this in extensions/ql-vscode/.storybook/preview.ts, though I couldn't see a way for us to share this code and avoid duplication. Although preview.ts can import code from src (as evidenced by VsCodeApi) it doesn't do so automatically and hence this declare global doesn't get picked up. I'm not too concerned about it though 🤷🏼

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 review from a team as code owners February 7, 2024 17:46
Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

Thanks!

(window as any).CSS = {
supports: jest.fn().mockResolvedValue(false),
};
window.CSS.supports = jest.fn().mockResolvedValue(false);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this equivalent in all cases? I think it depends on the implementation of JSDOM, but this assumes that window.CSS already exists and doesn't overwrite any other properties in window.CSS. I think that's fine here since all tests pass, but that means we are now changing the behavior of this property.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That is a good point that it is a different behaviour from before. I forgot/missed that when I wrote up the PR description.

When I just changed the type this line was producing an error because the CSS object contains other fields so just overwriting it with { supports } wasn't satisfying the full type. That's why I changed it to just overwrite the supports field instead of the entire CSS object.

As you say, since it was passing before and also passing now, I suppose it doesn't matter. The CSS field exists already, and apparently we don't use any other fields from it because it was ok when we overwrote it to a nearly-empty object.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think if any of these assumptions change in the future then it'll produce an error that we'll notice, so I'm inclined to just leave this as it is and not worry about it 🤷🏼

@robertbrignull robertbrignull merged commit 3e716e9 into main Feb 8, 2024
@robertbrignull robertbrignull deleted the robertbrignull/VsCodeWindow branch February 8, 2024 10:32
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.

2 participants