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

Attribute stringification #11177

Closed
zdm opened this issue Jun 20, 2024 · 16 comments · Fixed by #11182
Closed

Attribute stringification #11177

zdm opened this issue Jun 20, 2024 · 16 comments · Fixed by #11182
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. scope: custom elements

Comments

@zdm
Copy link

zdm commented Jun 20, 2024

Vue version

3.4.28

Link to minimal reproduction

http://example.com

Steps to reproduce

In vue 3.4.28 in @vue/runtime-dom/runtime-dom.esm-bundler.js line 587 you added attribute stribgification for non-boolean attrs:

...
el.setAttribute(key, isBoolean ? "" : String(value));
...

This is not correct and leads to errors.
Attribute can be not a string, and this is not prohibited.
We are using objects for some attrs.

What is expected?

Is it possible to revert this patch?

What is actually happening?

Please, see above.

System Info

Please, see above.

Any additional comments?

No response

@jh-leong
Copy link
Member

Invalid reproduction, could you please provide an actual reproduction?

@zdm
Copy link
Author

zdm commented Jun 20, 2024

yes, give me 2 hours, don't close issue pls

@jh-leong

This comment was marked as resolved.

@zdm
Copy link
Author

zdm commented Jun 20, 2024

I am using ExtJS web components:

<ext-grid :store="store" ... />

where store is instance of Store class.

@zdm
Copy link
Author

zdm commented Jun 20, 2024

Should I create and deploy reproduction code?
I think the problem is clear.

@LinusBorg
Copy link
Member

LinusBorg commented Jun 20, 2024

Should I create and deploy reproduction code? I think the problem is clear.

The problem is not really clear. Because the line you point to can't really be the problem on its own, here's why:

HTML Attributes are always converted to strings. That's part of the spec, and you can see a demo of this behaviour in this playground where I use vanilla DOM APIs to set an attribute to an obejct value without first converting it to a string manually. When you check the console, the attribute's value will be [object Object].

The patched line you point to just makes sure that stringification works in a special case where the value is a Symbol - which also doesn't apply to your situation.

So clearly, something else must be going on, maybe some edge case or interaction with the way these extJS web components handle attributes/dom properties? Usually, web components expose all their attributes as DOM properties as well, which in turn ensures that Vue doesn't set them as attributes, but as DOM properties (see this check), but as that part of the code wasn't affected by 3.4.28, this also doesn't look related

So yes, we need code from you that we can run to reproduce the actual problem in a running app. If you can reproduce it in the playground, that's fine. If you need to provide a minimal github repo, that's also cool.

@zdm
Copy link
Author

zdm commented Jun 20, 2024

It works when I replace String(value) with just value in your package source code.
The error is related exactly to this change and attrs are not converted to the strings by default.
ExtJS process attrs after vue, so the problem should be fixed in vue.

@LinusBorg
Copy link
Member

LinusBorg commented Jun 20, 2024

I still don't understand what the nature of the problem really is. We already established that the value will be stringified either way. So apparently, the exJS component would re-convert the string into a store instance?

So do we have an issue where our implementation does stringify that store object differently than the browser would?

As I explained, the change we did was to address an existing issue. If it causes a different issue for your situation, we need to understand why and how it causes an issue in order to find the best way to solve it without reverting the current fix, optimally.

We need your help to get to that understanding.

@LinusBorg
Copy link
Member

LinusBorg commented Jun 20, 2024

Okay, addendum: I think I can guess what the specific issue is:

the extJS behaviour relies on the fact that the browser would call toString on objects passed to ``setAttrtibute()`. Likely, this ExtJS store provides this method that would serialize the store into a string which the web component can re-convert into a Store instance (bad web component design breaking with best practices, but yeah ...).

String(store) doesn't do that. Turns out it does.... hm.

So a possible solution for keeping both the current fix and solve the reported issue might be:

} else {
    // attribute value is a string https://html.spec.whatwg.org/multipage/dom.html#attributes
-  el.setAttribute(key, isBoolean ? '' : String(value))
+  el.setAttribute(key, isBoolean ? '' : typeof value === 'symbol' ? : String(value) : value)
}

@zdm
Copy link
Author

zdm commented Jun 20, 2024

The problem is that store is the instance of the class, it is not a plain object, and can't be stringified / parsed from json.

I think your bugfix is not correct, you should not modify attrs values, or strictly limit the scope, where you do this.
If you need to convert Symbols to strings in some specific cases - you need to do this for your cases only, not for all cases.
At lease you can add type check to make sure, that attr value is a Symbol.

But most correct way - is to pass already stringified Sylmols to attrs, where you need it.

@zdm
Copy link
Author

zdm commented Jun 20, 2024

Yes. the code, you posted above, will work.

@LinusBorg
Copy link
Member

LinusBorg commented Jun 20, 2024

The problem is that store is the instance of the class, it is not a plain object, and can't be stringified / parsed from json.

But that's the part I don't understand. I just demonstrated to you that attributes will always be stringified by the browser. So ExtJS must re-parse a string into a Store Class instance.

But maybe that's something for users of those components to worry about.

We should add the patch I proposed either way.

@zdm
Copy link
Author

zdm commented Jun 20, 2024

  1. vue pass object as attribute value to the custom web element.
  2. ext component stores attr value in the property, if type of value is object.

@LinusBorg
Copy link
Member

LinusBorg commented Jun 20, 2024

I don't understand that explanation given what I demonstrated in the playground, because it would look like this:

  1. vue pass object as attribute value to the custom web element.
  2. browser converts object to string
  3. ext component stores attr value in the property, if type of value is object, which it is not, because it's a string.

@zdm
Copy link
Author

zdm commented Jun 20, 2024

  1. you pass object as attr value.
  2. extjs has handler for set attribute value operation, it called before browser. If value is object - extjs stores it in property instead of attribute.

@LinusBorg
Copy link
Member

Ah, I see, thanks. So it overrides the default implementation setAttribute. that's an .... interesting approach.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. scope: custom elements
Projects
None yet
3 participants