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

fix(ssr): remove dev-only accessor warnings #5135

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nolanlawson
Copy link
Collaborator

Details

This is a step towards fixing #5134 and #5123. It removes some dev-only accessors when running in SSR mode.

The problem here is multi-fold. First off, this filter:

!(propName in HTMLElementPrototype) &&

...basically does nothing in SSR, because HTMLElementPrototype is empty:

export const HTMLElementConstructor: typeof HTMLElement =
typeof HTMLElement !== 'undefined' ? HTMLElement : (function () {} as any);

So this causes us to always add dev-only accessors for e.g. title, tabIndex, etc.

Next up, the renderer does an in check here:

if (propName in node) {

This is always true for title, tabIndex, etc., since the accessors were added in dev mode. So this leads to title/tabIndex/etc. differing between SSR and CSR (or between prod mode and dev mode).

In general, I'm starting to wonder if these dev-only accessors are even worth it anymore – they've caused a lot of headaches, and the only value is that you get a nice warning when you forget to add an @api. But for now we can at least disable them in SSR.

Does this pull request introduce a breaking change?

  • 😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • 🤞 No, it does not introduce an observable change.

GUS work item

@nolanlawson nolanlawson requested a review from a team as a code owner January 14, 2025 00:28
@@ -1,6 +1,6 @@
<x-component>
<template shadowrootmode="open">
<x-child aria-activedescendant="foo" aria-atomic="foo" aria-autocomplete="foo" aria-busy="foo" aria-checked="foo" aria-colcount="foo" aria-colindex="foo" aria-colspan="foo" aria-controls="foo" aria-current="foo" aria-describedby="foo" aria-details="foo" aria-disabled="foo" aria-errormessage="foo" aria-expanded="foo" aria-flowto="foo" aria-haspopup="foo" aria-hidden="foo" aria-invalid="foo" aria-keyshortcuts="foo" aria-label="foo" aria-labelledby="foo" aria-level="foo" aria-live="foo" aria-modal="foo" aria-multiline="foo" aria-multiselectable="foo" aria-orientation="foo" aria-owns="foo" aria-placeholder="foo" aria-posinset="foo" aria-pressed="foo" aria-readonly="foo" aria-relevant="foo" aria-required="foo" aria-roledescription="foo" aria-rowcount="foo" aria-rowindex="foo" aria-rowspan="foo" aria-selected="foo" aria-setsize="foo" aria-sort="foo" aria-valuemax="foo" aria-valuemin="foo" aria-valuenow="foo" aria-valuetext="foo" exportparts="foo" role="foo">
<x-child accesskey="foo" aria-activedescendant="foo" aria-atomic="foo" aria-autocomplete="foo" aria-busy="foo" aria-checked="foo" aria-colcount="foo" aria-colindex="foo" aria-colspan="foo" aria-controls="foo" aria-current="foo" aria-describedby="foo" aria-details="foo" aria-disabled="foo" aria-errormessage="foo" aria-expanded="foo" aria-flowto="foo" aria-haspopup="foo" aria-hidden="foo" aria-invalid="foo" aria-keyshortcuts="foo" aria-label="foo" aria-labelledby="foo" aria-level="foo" aria-live="foo" aria-modal="foo" aria-multiline="foo" aria-multiselectable="foo" aria-orientation="foo" aria-owns="foo" aria-placeholder="foo" aria-posinset="foo" aria-pressed="foo" aria-readonly="foo" aria-relevant="foo" aria-required="foo" aria-roledescription="foo" aria-rowcount="foo" aria-rowindex="foo" aria-rowspan="foo" aria-selected="foo" aria-setsize="foo" aria-sort="foo" aria-valuemax="foo" aria-valuemin="foo" aria-valuenow="foo" aria-valuetext="foo" dir="foo" draggable="foo" exportparts="foo" id="foo" lang="foo" role="foo" spellcheck="foo" tabindex="foo" title="foo">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is still not 100% right, but it's at least closer to what engine-dom does. What it's still missing is that, in the browser, there is some built-in massaging here – draggable for example should be massaged to "true".

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.

1 participant