fix(elements): detect matchesSelector prototype without IIFE by CaerusKaru · Pull Request #37799 · angular/angular

CaerusKaru · 2020-06-27T21:41:53Z

Although in SSR we patch the global prototypes with DOM globals
like Element and Node, this patch does not occur before the
matches function is called in Angular Elements. This is similar
to the behavior in @angular/upgrade.

Fixes #24551

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: 24551

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

gkalpak

I left a couple of comments. Otherwise lgtm.

Not directly related to this PR:

It seems we have similar (but not 100% the same) code in @angular/animations' shared.ts and @angular/upgrade's downgrade_component_adapter.ts. I wonder if it would make sense to have a shared (internal) utility package for such helpers.

Also, these helpers are not 100% the same. It is hard to tell whether the differences account for slightly different needs of each package and how the helpers are used or whether we should align all implementations 😕

petebacondarwin

Reviewed-for: size-tracking

Not going to block the merge. I guess 600 bytes is not a killer.

But I think it would be good to consider either (or both) of the following:

Make the _matches code easier to understand
Make the _matches code smaller

It is likely that these two are in opposition to each other. But I feel that as it stands we have the worst of both scenarios. I feel that it could be smaller or simpler...

Another option, as @gkalpak suggested, is to move this to a shared function, since I imagine that elements is rarely used without animations, in which case we are getting hit twice for this logic. Both of these have core as peer dependency, so it could just be moved there an exported "internally".

petebacondarwin · 2020-07-17T15:02:32Z

I guess you are using {}.toString.call() because process could feasibly be null?

petebacondarwin · 2020-07-17T15:07:25Z

This logic seems super complex. Is it to try to keep the code-size down or something else?

I guess it is just a copy from the animations library.

I also find this to be super complex.

jelbourn

Would it be possible to add this shim exclusively to server environments rather than always baking it into the client-side lib?

alan-agius4

Can we change this implementation to something like:

angular/packages/upgrade/src/common/src/downgrade_component_adapter.ts

Lines 291 to 300 in d1ea1f4

    
           let _matches: (this: any, selector: string) => boolean; 
        
           function matchesSelector(el: any, selector: string): boolean { 
        
             if (!_matches) { 
        
               const elProto = <any>Element.prototype; 
        
               _matches = elProto.matches || elProto.matchesSelector || elProto.mozMatchesSelector || 
        
                   elProto.msMatchesSelector || elProto.oMatchesSelector || elProto.webkitMatchesSelector; 
        
             } 
        
             return el.nodeType === Node.ELEMENT_NODE ? _matches.call(el, selector) : false; 
        
           }

it's much simpler, and should solve the issue since it's not using an IIFE.

Comments addressed.

Although in SSR we patch the global prototypes with DOM globals like Element and Node, this patch does not occur before the matches function is called in Angular Elements. This is similar to the behavior in @angular/upgrade. Fixes angular#24551

atscott · 2020-10-09T15:18:25Z

presubmit

Although in SSR we patch the global prototypes with DOM globals like Element and Node, this patch does not occur before the matches function is called in Angular Elements. This is similar to the behavior in @angular/upgrade. Fixes #24551 PR Close #37799

vinothbabu · 2020-10-14T03:50:14Z

Does this commit fix the createCustomElement issue in SSR?

gkalpak · 2020-10-14T12:30:22Z

What is the createCustomElement() issue? 😕

vinothbabu · 2020-10-14T12:38:36Z

What is the createCustomElement() issue? 😕

Angular Element is not available in SSR. (Below is the issue ticket).
#24551

gkalpak · 2020-10-14T13:11:51Z

Yes, this PR should have fixed #24551.

vinothbabu · 2020-10-14T13:13:55Z

Yes, this PR should have fixed #24551.

Wow, that's a great news. We can use custom-elements in SSR which was a road block and which release we are making it?

gkalpak · 2020-10-14T13:55:50Z

It will be in the next 10.1.x release (10.1.6 - assuming there are going to be more v10 releases before v11) and the next 11.x release (11.0.0-next.6).

angular-automatic-lock-bot · 2020-11-14T16:23:12Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

CaerusKaru added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release area: elements Issues related to Angular Elements labels Jun 27, 2020

ngbot Bot modified the milestone: needsTriage Jun 27, 2020

googlebot added the cla: yes label Jun 27, 2020

pullapprove Bot requested a review from andrewseguin June 27, 2020 21:42

CaerusKaru requested a review from gkalpak June 27, 2020 21:42

CaerusKaru force-pushed the adam/el branch 4 times, most recently from 11a4b48 to dad9f6e Compare June 27, 2020 22:36

gkalpak approved these changes Jun 29, 2020

View reviewed changes

Comment thread packages/elements/src/utils.ts Outdated

Comment thread packages/elements/src/utils.ts Outdated

Comment thread packages/elements/src/utils.ts Outdated

gkalpak 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 Jul 13, 2020

CaerusKaru force-pushed the adam/el branch from dad9f6e to d7a5332 Compare July 15, 2020 04:19

CaerusKaru 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 Jul 15, 2020

CaerusKaru removed the request for review from andrewseguin July 15, 2020 04:20

CaerusKaru force-pushed the adam/el branch from d7a5332 to b96c1fb Compare July 15, 2020 04:42

pullapprove Bot requested review from IgorMinar, alxhub, jelbourn, petebacondarwin and pkozlowski-opensource July 15, 2020 04:42

petebacondarwin approved these changes Jul 17, 2020

View reviewed changes

pullapprove Bot removed the request for review from IgorMinar July 23, 2020 03:34

jelbourn reviewed Jul 28, 2020

View reviewed changes

pullapprove Bot requested review from AndrewKushnir and jelbourn July 28, 2020 17:03

alan-agius4 reviewed Aug 9, 2020

View reviewed changes

Comment thread packages/elements/src/utils.ts Outdated

alan-agius4 previously requested changes Aug 9, 2020

View reviewed changes

gkalpak requested review from alan-agius4 and removed request for AndrewKushnir, IgorMinar, alan-agius4, jelbourn and pkozlowski-opensource October 8, 2020 17:47

gkalpak added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 8, 2020

CaerusKaru force-pushed the adam/el branch from c14d8cf to 6f75d5b Compare October 8, 2020 17:49

gkalpak added the action: merge The PR is ready for merge by the caretaker label Oct 8, 2020

atscott closed this in bf717b1 Oct 9, 2020

CaerusKaru deleted the adam/el branch October 10, 2020 03:54

gkalpak mentioned this pull request Nov 2, 2020

build(docs-infra): introduce prerendering for angular.io app's index.html and 404.html angularathens/2nd-workshop-angular-contrib#10

Open

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Nov 14, 2020

	let _matches: (this: any, selector: string) => boolean;

	function matchesSelector(el: any, selector: string): boolean {
	if (!_matches) {
	const elProto = <any>Element.prototype;
	_matches = elProto.matches \|\| elProto.matchesSelector \|\| elProto.mozMatchesSelector \|\|
	elProto.msMatchesSelector \|\| elProto.oMatchesSelector \|\| elProto.webkitMatchesSelector;
	}
	return el.nodeType === Node.ELEMENT_NODE ? _matches.call(el, selector) : false;
	}

Conversation

CaerusKaru commented Jun 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

alan-agius4 Aug 9, 2020

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alan-agius4 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atscott commented Oct 9, 2020

Uh oh!

vinothbabu commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkalpak commented Oct 14, 2020

Uh oh!

vinothbabu commented Oct 14, 2020

Uh oh!

gkalpak commented Oct 14, 2020

Uh oh!

vinothbabu commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkalpak commented Oct 14, 2020

Uh oh!

angular-automatic-lock-bot Bot commented Nov 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

CaerusKaru commented Jun 27, 2020 •

edited

Loading

alan-agius4 left a comment •

edited

Loading

vinothbabu commented Oct 14, 2020 •

edited

Loading

vinothbabu commented Oct 14, 2020 •

edited

Loading