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

Web Component custom attributes missing when using Preact #2860

Closed
ConradSollitt opened this issue Dec 12, 2020 · 10 comments
Closed

Web Component custom attributes missing when using Preact #2860

ConradSollitt opened this issue Dec 12, 2020 · 10 comments

Comments

@ConradSollitt
Copy link

Greetings, I've found a possible issue when using Preact with Web Components (Custom Elements) where custom HTML attributes are not being assigned to the element when rendered by Preact. Originally I had created this as a React demo to make sure the Components work with React so the code is almost identical except the Preact version loads Preact instead of React of course.

Alternatively perhaps the error is with the Web Component and React is just handling it but so far in my research everything with the Web Component seems good and it works well with regular HTML.

Reproduction

https://codepen.io/conrad-sollitt/pen/JjREjQg

Preact Version for the Demo

<script src="https://unpkg.com/[email protected]/dist/preact.min.js"></script>

<!-- Page also includes this so it works with Preact DevTools -->
<script src="https://unpkg.com/[email protected]/debug/dist/debug.umd.js"></script>
<script src="https://unpkg.com/[email protected]/devtools/dist/devtools.umd.js"></script>

Original Demo (Preact)
https://www.dataformsjs.com/examples/web-components-with-preact.htm

Based on React Version that Works
Both files share the same JSX
https://www.dataformsjs.com/examples/web-components-with-react.htm

Steps to reproduce

  • Use either the CodePen or original link I provided and click the Data Page from the top nav.
  • It should show the error below (Chrome Version 87) and in DevTools the attribute [url] is indeed missing.

image

Expected Behavior

  • Here is the React Version (link above), when you click on the same Data Page it works because the the url custom HTML attribute is included.

image

Additional Notes

HTML Source for the Demo
https://github.com/dataformsjs/dataformsjs/blob/master/examples/web-components-with-preact.htm

JSX Source for the Demo
https://github.com/dataformsjs/dataformsjs/blob/master/examples/web-components-with-react.jsx

Source for the <json-data> Web Component
https://github.com/dataformsjs/dataformsjs/blob/master/js/web-components/json-data.js
The minified version is simply minified using Terser and not compiled down to ES5 or earlier versions of JS

Source for working <markdown-content> Web Component
https://github.com/dataformsjs/dataformsjs/blob/master/js/web-components/markdown-content.js
Intresting the Markdown page is working and it too uses a url attribute
Both files have a similar layout and:

class {name} extends HTMLElement ...
  static get observedAttributes() {
    return ['url', '..'];
  }

I'll still do some more work to see if I can track down the issue myself.

@ConradSollitt ConradSollitt changed the title Web Component Custom Attributes Missing when using Preact Web Component Custom Attributes missing when using Preact Dec 12, 2020
@ConradSollitt ConradSollitt changed the title Web Component Custom Attributes missing when using Preact Web Component custom attributes missing when using Preact Dec 12, 2020
@ConradSollitt
Copy link
Author

I know what the issue is now and how to fix it (both my code and Preact) so I'm updating my Web Component and the main link (not the CodePen one) will likely be fixed later today.

It seems an edge case issue and since Web Components are not really very popular compared to using Preact/React Components so I think it's something that could be just be documented in case it's not going to be supported (might add to much code or slow down performance too much).

Condition that causes the error

// Only getter is defined
get url() { return this.getAttribute('url'); }

Fix for Web Component Developers

// Define both getter and setter
get url() { return this.getAttribute('url'); }
set url(newValue) { return this.setAttribute('url', newValue); }

How this works in Preact

https://github.com/preactjs/preact/blob/master/src/diff/props.js#L107

// At lines 116 and 117 in the current master branch the following condition evaluates
// `true` if only a getter is defined so the result is element property gets set 
// and not the HTML Attribute:
    !isSvg &&
    name in dom
) {
    dom[name] = value == null ? '' : value;

Possible Fix

// Ideal Case - Adding only one line
    !isSvg &&
    name in dom &&
    Object.getOwnPropertyDescriptor(dom.__proto__, name).set !== undefined
) {
    dom[name] = value == null ? '' : value;

// Worst Case - multiple files would need to be updated extra code needed, quick pseudo example:
// [diff/index.js] - new `isWebComponent` where `isSvg` is called
isSvg = newVNode.type === 'svg' || isSvg;
isWebComponent = newVNode.type.includes('-') || (newProps && typeof newProps.is === 'string')

// Then in [/diff/props.js] `isWebComponent` would be passed and more logic used
    !isSvg &&
    ((!isWebComponent && name in dom)
     || (isWebComponent && name in dom && Object.getOwnPropertyDescriptor(dom.__proto__, name).set !== undefined))
) {
    dom[name] = value == null ? '' : value;

While the ideal case listed above is small code change I would expect it to require a lot of code for basic unit testing and I'm not certain it wouldn't affect regular elements (quick test I did showed it would not in Chrome) but to be certain a change like this would require significant testing and manually with many different browsers on different devices (example widely used older Chromium Browsers or Safari which have partial Web Component Support).

Also as to why this works with React and not Preact the related React Code is here:
https://github.com/facebook/react/blob/master/packages/react-dom/src/client/DOMPropertyOperations.js#L136

It ends up calling this which is significant in size:
https://github.com/facebook/react/blob/master/packages/react-dom/src/shared/DOMProperty.js#L207

If Preact maintainers would be willing to considering handling getter only properties I would be happy to help out in making the change, unit tests, or even help testing with different mobile devices; granted doesn't seem like this will be on anyone's priority list.

@ConradSollitt
Copy link
Author

Final follow up unless anyone respond with questions.

// This earlier example above:
    !isSvg &&
    name in dom &&
    Object.getOwnPropertyDescriptor(dom.__proto__, name).set !== undefined
) {
    dom[name] = value == null ? '' : value;

// Would need to be more like this (however optimized to avoid multiple calls using `Object.get...`)
    !isSvg &&
    name in dom &&
    Object.getOwnPropertyDescriptor(Object.getPrototypeOf(dom), name).set !== undefined &&
    Object.getOwnPropertyDescriptor(Object.getPrototypeOf(dom), name).writable !== false
) {
    dom[name] = value == null ? '' : value;

I see that the release builds of Preact do not include "use strict"; so perhaps the error I originally described would have been caught if I used webpack or node tools. If Preact did include "use strict"; then this line dom[name] = value == null ? '' : value; would have thrown an error from Preact.

@ConradSollitt
Copy link
Author

Going to close out this issue I opened. I think it's a rare case and not worth adding extra code to Preact for. Vue 3 has the same behavior as well when using Web Components.

If anyone is reading this with the same issue just make sure you define both a getter and setter when the HTML attribute and DOM Property have the same name for an Custom Element.

@marvinhagemeister
Copy link
Member

Good find! I'm curious if we should switch and add "use strict"; at the top of our code that's packaged for npm. That's a difficult issue to track down, much respect to ya! If I'm understanding it correctly, the problem is that the DOM properties are not reflected back to the attributes, right?

Regarding the getOwnPropertyDescriptor: I'm worried that this is expensive. Don't have any current numbers to back that up, something worth to check, but I'm willing to bet that there must be a better way.

@ConradSollitt
Copy link
Author

Thanks @marvinhagemeister

Yeah I agree with getOwnPropertyDescriptor which is why I closed out this issue but after reading our comment on "use strict"; I think it's definitely worth looking into and considering.

I did a quick check of several of the most widely used Libraries and found the following list are all using use strict:

  • jQuery
  • Bootstrap
  • React
  • Vue (both 2 and 3)

Here is a comment from the latest version of jQuery near the top:

https://code.jquery.com/jquery-3.5.1.js

// Edge <= 12 - 13+, Firefox <=18 - 45+, IE 10 - 11, Safari 5.1 - 9+, iOS 6 - 9.1
// throw exceptions when non-strict code (e.g., ASP.NET 4.5) accesses strict mode
// arguments.callee.caller (trac-13335). But as of jQuery 3.0 (2016), strict mode should be common
// enough that all such attempts are guarded in a try block.
"use strict";

You are understanding issue correctly in that DOM properties are not reflected back to the attributes. Here are a few links in case you want to take a glance:

Snippet from the first link:

export class MyCustomElement extends HTMLElement {
  ...
  
  get disabled() {
    return this.hasAttribute('disabled');
  }
  
  set disabled(val) {
    if(val) {
      this.setAttribute('disabled', val);
    } else {
      this.removeAttribute('disabled');
    }
  }
}

static get observedAttributes() {
  return ['disabled'];
}

attributeChangedCallback(name, oldValue, newValue) {
  this.disabled = newValue;  
}

As you can see it's a lot of extra code to make the attribute always match the value of the property. I've been experimenting with Web Components recently in some production apps and found that all the extra code results in myself (and I would assume other developers) not adding setter functions or other features unless it's really needed. In my original example it made sense to originally have a getter but I didn't realize the need for a setter at first so I didn't add one.

Related to how Vue 3 handles this it provides a warning (shown in screenshow below) if using the uncompressed build, however the error doesn't show up when using the compressed production build.

Uncompressed build: https://cdn.jsdelivr.net/npm/[email protected]/dist/vue.global.js

image

To see how it would be have with Preact I did some testing with and without use strict. I took the minimized production version and add it at the top.

image

When including https://unpkg.com/[email protected]/debug/dist/debug.umd.js this error shows if use strict is included but if the debug file is not included then no error shows up (which is a similar result to Vue 3).

image

I've researched performance in the past related to use strict and I know for some code it can increase performance (not sure if enough for an end user to notice though). Quick google result for some relevant links from Stack Overflow:

However I would be hesitant to add it to Preact 10 because it might break code for developers who use Preact. For Preact 11 I think it would be a good addition to add. Although now as I write this, I think it wouldn't have any impact on 99%+ of apps (just guessing), but at the very least it would need to be on a minor release (e.g.: 10.6.0) rather than a patch release (10.5.8 if published).

@havran
Copy link

havran commented Nov 11, 2021

Thank you for finding. I just face same, (or similar), problem in NextJS (12.0.4-canary.5 currently).

I use custom element (build in VueJS 2) and for production I replace React with Preact.

When I load page first time, element render without problems.
When I navigate from page, which contain element, to different page and then navigate back, element lost some attributes - I realize only 'single word' attributes are missing.

When I use React, problem go away.

Any advice how to solve this issue? Thanks.

@ConradSollitt
Copy link
Author

@havran
Can you provide an example or some code? Sounds like it might be an issue with NextJS.

Here is a SPA that uses Preact with Web Components. On the "Data Example" page it uses a web component with single word attribute <json-data url="{url}">. When you click off and on the page it renders correctly. For the SPA functionality React Router is used.

Demo
https://dataformsjs.com/examples/web-components-with-preact.htm#/

Source HTML
https://github.com/dataformsjs/dataformsjs/blob/master/examples/web-components-with-preact.htm

Source JSX
The JSX is shared by both React and Preact Demos
https://github.com/dataformsjs/dataformsjs/blob/master/examples/web-components-with-react.jsx

@havran
Copy link

havran commented Nov 12, 2021

@ConradSollitt Hi, thanks for answer.

I try recreate same bug, with simple web component and basic NextJS app, which I create, but without success. Simple component work.

Seems like problem is related with component which we use, because when I try use same component, then I get same problem.

When I found some more, I add this info here. Thanks again.

@weaintplastic
Copy link

Preact's behavior of not rendering attributes to custom elements creates issues in our isomorphic environment.

We pre-render web components by hydrating them on the server-side. In a nutshell, web components are rendered with light DOM and scoped CSS instead of Shadow DOM. This way we can guarantee that our web components are displayed on the client-side before the application is available.

The server-side hydration of web components operates on a DOM that was created using preact. Because of the missing attributes on custom elements, the hydration is missing meaningful instructions that otherwise would be available when added as attributes.

It seems that the React team stumbled upon this caveat while working on web component support. It would be great if this issue could be reopened to continue the discussion to set attributes rather than assigning property values for web components when rendered with Preact.

@trusktr
Copy link

trusktr commented Jun 20, 2024

Having explicit control over setting JS properties vs DOM attributes on elements would alleviate issues like this one in the OP because a user could elect to set an attribute instead of a JS property.

I've made a proposal to add a new test to custom-elements-everywhere to cover this, which would ensure that complying frameworks allow users to pick whether they set properties vs attributes:

And here's the issue I made to track the idea for Preact:

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

No branches or pull requests

5 participants