Skip to content

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

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.

Sign up for GitHub

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

Merged
robertbrignull merged 1 commit into from
Feb 8, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion extensions/ql-vscode/src/view/common/CodePaths/__tests__/CodePaths.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ describe(CodePaths.name, () => {

await userEvent.click(screen.getByText("Show paths"));

expect((window as any).vsCodeApi.postMessage).toHaveBeenCalledWith({
expect(window.vsCodeApi.postMessage).toHaveBeenCalledWith({
t: "showDataFlowPaths",
dataFlowPaths: {
codeFlows: createMockCodeFlows(),
Expand Down
8 changes: 3 additions & 5 deletions extensions/ql-vscode/src/view/jest.setup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,12 @@ Object.defineProperty(window, "matchMedia", {
});

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

@koesie10 koesie10 Feb 8, 2024

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

@robertbrignull robertbrignull Feb 8, 2024

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

@robertbrignull robertbrignull Feb 8, 2024

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 🤷🏼


// Store this on the window so we can mock it
(window as any).vsCodeApi = {
window.vsCodeApi = {
postMessage: jest.fn(),
setState: jest.fn(),
};

(window as any).acquireVsCodeApi = () => (window as any).vsCodeApi;
window.acquireVsCodeApi = () => window.vsCodeApi;
2 changes: 1 addition & 1 deletion extensions/ql-vscode/src/view/variant-analysis/__tests__/RepoRow.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ describe(RepoRow.name, () => {
);
});

expect((window as any).vsCodeApi.postMessage).toHaveBeenCalledWith({
expect(window.vsCodeApi.postMessage).toHaveBeenCalledWith({
t: "requestRepositoryResults",
repositoryFullName: "octodemo/hello-world-1",
});
Expand Down
11 changes: 11 additions & 0 deletions extensions/ql-vscode/src/view/vscode-window.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import type { VsCodeApi } from "./vscode-api";

declare global {
interface Window {
CSS: {
supports: () => Promise<boolean>;
};
vsCodeApi: VsCodeApi;
acquireVsCodeApi: () => VsCodeApi;
}
}