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

[svelte-native] broken custom components with 'src' attribute #6575

Closed
farfromrefug opened this issue Jul 26, 2021 · 12 comments · Fixed by #6580
Closed

[svelte-native] broken custom components with 'src' attribute #6575

farfromrefug opened this issue Jul 26, 2021 · 12 comments · Fixed by #6580
Labels

Comments

@farfromrefug
Copy link

Describe the bug

This commit ecbd96a broke svelte-native.
The reason is that we dont have a element.
I think the handle of src attribute should not happen on foreign namespace.

Reproduction

create a simple svelte-native project with a listview and image component (which has a src attribute).
It will crash as you scroll with that error:

System.err: Calling js method onBindViewHolder failed
System.err: TypeError: No known component for element a.
System.err: 
System.err: StackTrace:
System.err: createElement(file: node_modules/.pnpm/[email protected]_6638bbb868bf526fdb8641bdfec1676a/node_modules/svelte-native/dom/index.js:321:0)

Logs

No response

System Info

Nativescript 8.x
Svelte 3.40.2

Severity

blocking an upgrade

@Conduitry
Copy link
Member

Yeah this sounds reasonable. I think it would make sense to add a check to

that this.parent.node.namespace !== namespaces.foreign.

@Conduitry Conduitry added the bug label Jul 26, 2021
@farfromrefug
Copy link
Author

@Conduitry i was wondering if should not even put it in the is_src setter. I am afraid it might happen again in the future from other places. Ensuring that is_src is always false in svelte-native could enforce it never happens again.

@Conduitry
Copy link
Member

Yeah that would be another possibility. I don't have particularly strong opinions one way or the other. It looks like the only other thing that would currently affect would be should_cache, which might actually be desirable.

@baseballyama
Copy link
Member

Hi.

I fixed this issue.
But I want to confirm what I did is correct or not.

What I did

Already @farfromrefug mentioned that I can fix it after modifying the way of assign value to is_src like below.

this.is_src = this.name === 'src'; // TODO retire this exception in favour of https://github.com/sveltejs/svelte/issues/3750

this.is_src = this.name === 'src' && this.parent.node.namespace !== namespaces.foreign;

Result

But after fixing up, still, an error occurred in the svelte native.
Because is_src assinged true if parent node name is <image> tag and namespace is http://www.w3.org/2000/svg.

My Opinion

I think this.is_src should be true if an attribute name is src and namespaces is only HTML(http://www.w3.org/1999/xhtml).
But I don't have confidence due to my knowledge.
Therefore I want to confirm what I did is correct or not.

this.is_src = this.name === 'src' && (!this.parent.node.namespace || this.parent.node.namespace === namespaces.html);

Notice: Already I executed all existing tests after change the code and these were passed.

Survey MEMO

I've investigated the possibility that this.is_src may be assigned true by each namespace.

  • foreign (https://svelte.dev/docs#svelte_options)

It comes if code is like <svelte:options tag="my-custom-element"/>.
Always this.is_src should be false.

  • html (http://www.w3.org/1999/xhtml)
    This is normal HTML.
    this.is_src should be true if attribute name is src.

  • mathml (http://www.w3.org/1998/Math/MathML)
    We can't judge src attribute should come URL.
    So always this.is_src should be false.

  • svg (http://www.w3.org/2000/svg)
    refer: https://developer.mozilla.org/en-US/docs/Web/SVG/Element/image
    If use <image> inside <svg>, should use href attribute instead of src.
    So always this.is_src should be false.

  • xlink (http://www.w3.org/1999/xlink)
    This is XML (not HTML).
    We can't judge src attribute should come URL.
    So always this.is_src should be false.

  • xml (http://www.w3.org/XML/1998/namespace)
    This is XML (not HTML).
    We can't judge src attribute should come URL.
    So always this.is_src should be false.

  • xmlns (http://www.w3.org/2000/xmlns`)
    We can't judge src attribute should come URL.
    So always this.is_src should be false.

Thank you for reading!🎈

@farfromrefug
Copy link
Author

@baseballyama thanks a lot for looking at this. It seems good to me though i am really not an html expert

@Conduitry
Copy link
Member

Should <svelte:options namespace='foreign'/> make it so that the entire component has the foreign namespace, overriding the automatic switch inside an <svg> element in that component? (And would that be another path towards addressing this?)

@baseballyama
Copy link
Member

@Conduitry

Thank you for the suggestion!

Actually now <svg> tag's namespace which is inside https://svelte.dev/docs#svelte_options is http://www.w3.org/2000/svg if xmlns attribute is http://www.w3.org/2000/svg.

REPL: https://svelte.dev/repl/b7624c2a55dd4b6c8c2bd311a7184f0e?version=3.41.0
(Please check console log)

According to MDN, xmlns attribute is require for <svg>.
refer: https://developer.mozilla.org/en-US/docs/Web/SVG/Element/svg

Therefore, if xmlns attribute is not http://www.w3.org/2000/svg or not defined, a namespace should not change to http://www.w3.org/2000/svg automatically even inside of https://svelte.dev/docs#svelte_options.

So I think regarding <svg> namespace's implementation is already correct.
And the way of fixing this bug is only my PR at least for now.

@farfromrefug
Copy link
Author

@Conduitry sorry to bump this. I would like to help but it goes beyond my knowledge. Right we cant update svelte anymore

@baseballyama
Copy link
Member

@farfromrefug

Sorry for the reply lately. (Recently my full-time job is extremely busy. So I couldn't anything for Svelte and Kit...)
And I replied for the PR's comment and I will re-update if its necessary.
#6580

@farfromrefug
Copy link
Author

@baseballyama dont apologize ! Thank you for taking the time to look at this and for your PR.

@Conduitry
Copy link
Member

3.42.4 should now avoid the src optimization when in a non-html namespace. Thanks @baseballyama!

@farfromrefug
Copy link
Author

@baseballyama @Conduitry thanks a lot ! I ll test and report if any issue is remaining

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants