Skip to content

Improve type safety for dynamic stylesheet rules#288651

Merged
mjbvz merged 7 commits intomicrosoft:mainfrom
chetanr-25:fix-stylesheet-types
Apr 2, 2026
Merged

Improve type safety for dynamic stylesheet rules#288651
mjbvz merged 7 commits intomicrosoft:mainfrom
chetanr-25:fix-stylesheet-types

Conversation

@chetanr-25
Copy link
Copy Markdown
Contributor

This PR improves type safety when accessing dynamic stylesheet rules
by ensuring a consistent CSSRuleList return type.

No functional behavior is changed.

@vs-code-engineering
Copy link
Copy Markdown
Contributor

📬 CODENOTIFY

The following users are being notified based on files changed in this PR:

@bpasero

Matched files:

  • src/vs/base/browser/domStylesheets.ts

@chetanr-25
Copy link
Copy Markdown
Contributor Author

This is my first contribution — happy to make any requested changes. Thank you for reviewing!

@aeschli aeschli assigned mjbvz and bpasero and unassigned aeschli and mjbvz Jan 19, 2026
mjbvz
mjbvz previously approved these changes Jan 20, 2026
Copy link
Copy Markdown
Collaborator

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

Thanks!

@mjbvz mjbvz enabled auto-merge January 20, 2026 19:10
@vs-code-engineering vs-code-engineering bot added this to the January 2026 milestone Jan 20, 2026
bpasero
bpasero previously approved these changes Jan 20, 2026
auto-merge was automatically disabled January 20, 2026 23:07

Head branch was pushed to by a user without write access

@chetanr-25 chetanr-25 dismissed stale reviews from bpasero and mjbvz via 118e74b January 20, 2026 23:07
@chetanr-25
Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

@chetanr-25
Copy link
Copy Markdown
Contributor Author

chetanr-25 commented Jan 20, 2026 via email

@mjbvz mjbvz enabled auto-merge January 21, 2026 00:26

const emptyRules: CSSRule[] = [];

return {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'd prefer this just return undefined in these cases

And actually do we even need this helper function now? The 2 callers can be converted to style.sheet?.cssRules instead

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.

Agreed — removed the helper and updated both call sites to use
style.sheet?.cssRules directly. Thanks!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hey looks like this change didn't make it in yet, right?

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.

Yep, it’s approved but not merged yet. Let me know if I should do anything else or if it’s waiting on a maintainer.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sorry what I meant is that I don't think we need getDynamicStyleSheetRules at all now. I pushed a change that just inlines which simplifies the code quite a bit. Let me know if you don't think that will work

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.

The inlined changes look great and definitely simplify the code. I'm happy with this approach. Thanks for the help!

@chetanr-25
Copy link
Copy Markdown
Contributor Author

chetanr-25 commented Jan 21, 2026 via email

@bpasero bpasero assigned mjbvz and unassigned bpasero Jan 21, 2026
@bpasero bpasero requested a review from mjbvz January 21, 2026 07:16
@mjbvz mjbvz modified the milestones: January 2026, February 2026 Jan 27, 2026
@mjbvz mjbvz modified the milestones: 1.112.0, 1.113.0, Backlog Mar 16, 2026
@chetanr-25
Copy link
Copy Markdown
Contributor Author

Hi @mjbvz, I've updated the PR to remove the helper function and used optional chaining as suggested. I also just merged the latest main branch to keep it up to date. Ready for the tests to run and a final review whenever you have a moment! Thanks!

mjbvz and others added 2 commits April 1, 2026 22:30
@mjbvz
Copy link
Copy Markdown
Collaborator

mjbvz commented Apr 2, 2026

@chetanr-25 Please hold off on updating the branch. It means I have to re-approve the PR each time

@mjbvz mjbvz merged commit 351c056 into microsoft:main Apr 2, 2026
19 checks passed
@mjbvz mjbvz modified the milestones: Backlog, 1.115.0 Apr 2, 2026
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.

5 participants