[Glimmer2] Support id as alias to elementId#13478
[Glimmer2] Support id as alias to elementId#13478asakusuma wants to merge 1 commit intoemberjs:masterfrom
Conversation
|
LGTM /cc @krisselden @chancancode |
There was a problem hiding this comment.
This is the wrong place to do it IMO. I don't think we want to actually allow setting this.id on the instance – we have to rename the id arg to elementId on the way in.
There was a problem hiding this comment.
Renaming it in CurlyComponentMananger.create throws a Changing a view's elementId after creation is not allowed error. I don't quite understand the difference between doing it in CurlyComponentMananger.create vs where it is now in view_support. To me, both seem to be allowing id setting per instance. Which seems to be what's expected anyways.
There was a problem hiding this comment.
There is a big difference, in that we want to isolate all of normalizing of component props to one place. If this didn't work there there is something odd happening in the curly component manager that needs to be addressed.
There was a problem hiding this comment.
Sorry for the confusion – github attached the comment to one line above. I meant to say that we shouldn't have to do this.id || guidFor(this); at this point – because we should already have normalized it to elementId by now.
There was a problem hiding this comment.
We convert the args to a "creation props" pojo that we pass to Component.create(...). That pojo should contain elementId, not id.
There was a problem hiding this comment.
Figured out the problem. I didn't realize my alias code was getting run on update as well as creation. Seems to work fine now.
|
👍 LGTM. I'll merge this into #13490 since I refactored the params processing code |
|
☔ The latest upstream changes (presumably #13490) made this pull request unmergeable. Please resolve the merge conflicts. |
cc @chadhietala