fix(ivy): Ensure proper namespace is used to create elements in JIT #28144
Conversation
|
You can preview 6131b4a at https://pr28144-6131b4a.ngbuilds.io/. |
|
@benlesh it looks like you've created this PR on top of the master while it was in a state where "integration_test" was broken. Please rebase it from the latest master to make "integration_test" pass. Thank you. |
benlesh
force-pushed
the
FW-811-Align-Namespaces
branch
from
6131b4a
to
3c7b67d
Compare
benlesh
added
target: major
benlesh
force-pushed
the
FW-811-Align-Namespaces
branch
from
3c7b67d
to
5033202
Compare
|
You can preview 3c7b67d at https://pr28144-3c7b67d.ngbuilds.io/. |
|
You can preview 5033202 at https://pr28144-5033202.ngbuilds.io/. |
| return document.createElementNS(NAMESPACE_URIS[namespace], name); | ||
| // In cases where Ivy (not ViewEngine) is giving us the actual namespace, the look up by key | ||
| // will result in undefined, so we just return the namespace here. | ||
| return document.createElementNS(NAMESPACE_URIS[namespace] || namespace, name); |
There was a problem hiding this comment.
There are two other places where we look the namespace up in this file - setAttribute and removeAttribute. Can you update those too?
There was a problem hiding this comment.
Update based on offline discussion:
- add TODO comments to
setAttributeandremoveAttribute - remove fixme for test
|
@kara @benlesh I think we may also need to check how VE and Ivy handle the case when invalid/unsupported namespace is specified and align this behavior (and have a test to capture this). My understanding is that we'll see a difference after this change due to the fact that we return namespace as is in case it's not registered in |
|
What if some custom renderers do need the raw namespace text? Renderer is a public API, how could a breaking change being introduced directly? |
benlesh
force-pushed
the
FW-811-Align-Namespaces
branch
from
5033202
to
d81b8b3
Compare
| @@ -154,6 +156,8 @@ class DefaultDomRenderer2 implements Renderer2 { | |||
| setAttribute(el: any, name: string, value: string, namespace?: string): void { | |||
| if (namespace) { | |||
| name = `${namespace}:${name}`; | |||
| // TODO(benlesh): Ivy may cause issues here because it's passing around | |||
| // full URIs for namespaces, therefor this lookup will fail. | |||
There was a problem hiding this comment.
Same below:
| // full URIs for namespaces, therefor this lookup will fail. | |
| // full URIs for namespaces, therefore this lookup will fail. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Updates render to properly use the namespace URL that Ivy passes around rather than use it to look up the namespace URL ala ViewEngine.
Resolves FW-811