fix(elements): a couple of small fixes by gkalpak · Pull Request #36114 · angular/angular · GitHub
Skip to content

fix(elements): a couple of small fixes - #36114

Closed
gkalpak wants to merge 8 commits into
angular:masterfrom
gkalpak:fix-elements-inputs-before-upgrade
Closed

gkalpak wants to merge 8 commits into
angular:masterfrom
gkalpak:fix-elements-inputs-before-upgrade

Conversation

@gkalpak

@gkalpak gkalpak commented Mar 17, 2020 •

Copy link
Copy Markdown
Member

Several small fixes and refactorings. See individual commits for details.
This is an alternative to #31416 (with the feedback there addressed).

Jira issue: FW-2006

Fixes #30848.

NOTE:
The last commit is a fix to the public API. It should not affect users in any way (afaict), but we might still want to only merge that last commit to master-only. (The rest is good for both master and patch.)
Happy to move the last commit to a separate PR, if we decide it shouldn't go into patch.

UPDATE: The last commit has been removed. See #36114 (comment).

@pullapprove
pullapprove Bot requested a review from andrewseguin March 17, 2020 18:28
@gkalpak
gkalpak requested a review from IgorMinar March 17, 2020 18:29
@gkalpak gkalpak added area: elements Issues related to Angular Elements action: review The PR is still awaiting reviews from at least one requested reviewer PR target: TBD type: bug/fix refactoring Issue that involves refactoring or code-cleanup labels Mar 17, 2020
@ngbot ngbot Bot modified the milestone: needsTriage Mar 17, 2020
@gkalpak
gkalpak force-pushed the fix-elements-inputs-before-upgrade branch 2 times, most recently from 728f981 to 77ea12e Compare March 17, 2020 18:34
@mary-poppins

Copy link
Copy Markdown

@gkalpak
gkalpak force-pushed the fix-elements-inputs-before-upgrade branch 2 times, most recently from 016f230 to 04d17e1 Compare March 17, 2020 19:43
@gkalpak gkalpak added state: WIP and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 17, 2020
@mary-poppins

Copy link
Copy Markdown

You can preview 04d17e1 at https://pr36114-04d17e1.ngbuilds.io/.

@gkalpak
gkalpak force-pushed the fix-elements-inputs-before-upgrade branch from 04d17e1 to bb59493 Compare March 17, 2020 21:39
@mary-poppins

Copy link
Copy Markdown

You can preview bb59493 at https://pr36114-bb59493.ngbuilds.io/.

@gkalpak
gkalpak force-pushed the fix-elements-inputs-before-upgrade branch 9 times, most recently from b6d5b93 to cc8ec4c Compare March 18, 2020 18:25
@kara kara added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label May 19, 2020
@gkalpak gkalpak added target: patch This PR is targeted for the next patch release and removed target: major This PR is targeted for the next major release labels May 20, 2020
gkalpak added 8 commits May 20, 2020 11:10
…necting the element

`createCustomElements()` creates some getters/setters for properties
corresponding to component inputs that delegate to the
`NgElementStrategy`. However, it is not guaranteed that the element's
`NgElementStrategy` will have been created when these getters/setters
are called, because some polyfills (e.g. `document-register-element`) do
not call the constructor.

Previously, trying to get/set input properties before connecting the
element to the DOM (via `connectedCallback()`) would fail due to
`NgElementStrategy` not being created.

This commit ensures that the `NgElementStrategy` is always created
before used inside the input property getters/setters (similar to how it
is done for other methods of `NgElement`).

Mentioned in https://github.com/angular/angular/pull/31416/files#r300326698.
Previously, we had to check whether `NgElementStrategy` had been
instantiated before accessing it. This was tedious and error prone,
since it was easy to forget to add the check in new call sites.

This commit switches to using a getter, so that the check has to be
performed in one place and is transparent to call sites (including any
future ones).
- Remove imports that are not used.
- Remove private class properties that are not used outside of the
  constructor.
Previously, helper modules/components classes were declared even if the
tests were not run (because the environment did not support Custom
Elements for example).

This commit moves the declaration of the helpers inside the `describe()`
block, so they are not declared unnecessarily. This is in preparation of
adding more helpers that need to access variables declared inside the
`describe()` block.
Previously, the `TestStrategy` `NgElementStrategy` used in
`createCustomElement()` tests was created once and re-used in each test
(due to complications related to how `customElements.register()` works).
As a result, the `TestStrategy` instance's state (e.g. inputs) could be
polluted from previous tests and affect subsequent ones.

This commit ensures the strategy instance is reset before each test.
Previously, if an element started out as a regular `HTMLElement` (not a
Custom Element) and was later upgraded to a Custom Element, any
properties corresponding to component inputs that were set on the
element before upgrading it would not be captured correctly and thus not
reflected on the instantiated component.

This commit fixes it by ensuring that such properties are captured
correctly.

Jira issue: [FW-2006](https://angular-team.atlassian.net/browse/FW-2006)

Fixes angular#30848
Closes angular#31416
…t is not called

Previously, the correct behavior of Angular custom elements relied on
the constructor being called (and thus the `injector` property being
initialized). However, some polyfills (e.g. `document-register-element`)
do not call the constructor of custom elements, which resulted in the
`injector` property being undefined and the `NgElementStrategy` failing
to be instantiated.

This commit fixes it by being tolerant to the `injector` property being
undefined and falling back to the injector passed to the
`createCustomElement()` config.

NOTE:
We don't have proper tests exercising the situation where the
constructor is not called. For now this is tested using a Google
internal test suite (which is how this issue was caught).
This commit also adds a rudimentary unit test to emulate this situation.
@gkalpak
gkalpak force-pushed the fix-elements-inputs-before-upgrade branch from 1f32e73 to dda5898 Compare May 20, 2020 08:15
@gkalpak

gkalpak commented May 20, 2020

Copy link
Copy Markdown
Member Author

I have removed the public API commit, so this is good for master and patch. (I have updated the original comment.) I have also rebased and resolved conflicts.

@mary-poppins

Copy link
Copy Markdown

You can preview dda5898 at https://pr36114-dda5898.ngbuilds.io/.

@gkalpak gkalpak removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label May 20, 2020
@kara kara closed this in 327980b May 20, 2020
kara pushed a commit that referenced this pull request May 20, 2020
Previously, we had to check whether `NgElementStrategy` had been
instantiated before accessing it. This was tedious and error prone,
since it was easy to forget to add the check in new call sites.

This commit switches to using a getter, so that the check has to be
performed in one place and is transparent to call sites (including any
future ones).

PR Close #36114
kara pushed a commit that referenced this pull request May 20, 2020
- Remove imports that are not used.
- Remove private class properties that are not used outside of the
  constructor.

PR Close #36114
kara pushed a commit that referenced this pull request May 20, 2020
Previously, helper modules/components classes were declared even if the
tests were not run (because the environment did not support Custom
Elements for example).

This commit moves the declaration of the helpers inside the `describe()`
block, so they are not declared unnecessarily. This is in preparation of
adding more helpers that need to access variables declared inside the
`describe()` block.

PR Close #36114
kara pushed a commit that referenced this pull request May 20, 2020
Previously, the `TestStrategy` `NgElementStrategy` used in
`createCustomElement()` tests was created once and re-used in each test
(due to complications related to how `customElements.register()` works).
As a result, the `TestStrategy` instance's state (e.g. inputs) could be
polluted from previous tests and affect subsequent ones.

This commit ensures the strategy instance is reset before each test.

PR Close #36114
kara pushed a commit that referenced this pull request May 20, 2020
…ent (#36114)

Previously, if an element started out as a regular `HTMLElement` (not a
Custom Element) and was later upgraded to a Custom Element, any
properties corresponding to component inputs that were set on the
element before upgrading it would not be captured correctly and thus not
reflected on the instantiated component.

This commit fixes it by ensuring that such properties are captured
correctly.

Jira issue: [FW-2006](https://angular-team.atlassian.net/browse/FW-2006)

Fixes #30848
Closes #31416

PR Close #36114
kara pushed a commit that referenced this pull request May 20, 2020
…t is not called (#36114)

Previously, the correct behavior of Angular custom elements relied on
the constructor being called (and thus the `injector` property being
initialized). However, some polyfills (e.g. `document-register-element`)
do not call the constructor of custom elements, which resulted in the
`injector` property being undefined and the `NgElementStrategy` failing
to be instantiated.

This commit fixes it by being tolerant to the `injector` property being
undefined and falling back to the injector passed to the
`createCustomElement()` config.

NOTE:
We don't have proper tests exercising the situation where the
constructor is not called. For now this is tested using a Google
internal test suite (which is how this issue was caught).
This commit also adds a rudimentary unit test to emulate this situation.

PR Close #36114
kara pushed a commit that referenced this pull request May 20, 2020
…necting the element (#36114)

`createCustomElements()` creates some getters/setters for properties
corresponding to component inputs that delegate to the
`NgElementStrategy`. However, it is not guaranteed that the element's
`NgElementStrategy` will have been created when these getters/setters
are called, because some polyfills (e.g. `document-register-element`) do
not call the constructor.

Previously, trying to get/set input properties before connecting the
element to the DOM (via `connectedCallback()`) would fail due to
`NgElementStrategy` not being created.

This commit ensures that the `NgElementStrategy` is always created
before used inside the input property getters/setters (similar to how it
is done for other methods of `NgElement`).

Mentioned in https://github.com/angular/angular/pull/31416/files#r300326698.

PR Close #36114
kara pushed a commit that referenced this pull request May 20, 2020
Previously, we had to check whether `NgElementStrategy` had been
instantiated before accessing it. This was tedious and error prone,
since it was easy to forget to add the check in new call sites.

This commit switches to using a getter, so that the check has to be
performed in one place and is transparent to call sites (including any
future ones).

PR Close #36114
kara pushed a commit that referenced this pull request May 20, 2020
- Remove imports that are not used.
- Remove private class properties that are not used outside of the
  constructor.

PR Close #36114
kara pushed a commit that referenced this pull request May 20, 2020
Previously, helper modules/components classes were declared even if the
tests were not run (because the environment did not support Custom
Elements for example).

This commit moves the declaration of the helpers inside the `describe()`
block, so they are not declared unnecessarily. This is in preparation of
adding more helpers that need to access variables declared inside the
`describe()` block.

PR Close #36114
@angular-automatic-lock-bot

Copy link
Copy Markdown

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: elements Issues related to Angular Elements cla: yes refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release type: bug/fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Angular Elements doesn't bind existing property values to component inputs

8 participants