feat(cdk/testing): Allow custom `querySelectorAll` method #18178

mmalerba · 2020-01-15T16:22:34Z

This allows the user to drop in a shadow-piercing version of
querySelectorAll to handle components that use
ViewEncapsulation.ShadowDom

I've gotten a unit test working that uses kagekiri to demonstrate working with components that use ViewEncapsulation.ShadowDom.

It should be possible to get an e2e test working using the same library, but I had some difficulty getting it set up. The librariy's package.json specifies the common js module as the main. The e2e test kept loading that instead of the umd, and I wasn't sure how to configure it

crisbeto

LGTM

jelbourn · 2020-01-16T00:25:32Z

src/cdk/testing/kagekiri.d.ts

+ */
+
+declare module 'kagekiri' {
+  export function querySelectorAll(selector: string, root: Element);


Not also querySelector?

Nope, all the harness stuff runs off just qsa

jelbourn · 2020-01-16T00:30:54Z

src/cdk/testing/protractor/protractor-harness-environment.ts

    super(rawRootElement);
  }

  /** Creates a `HarnessLoader` rooted at the document root. */
-  static loader(): HarnessLoader {
-    return new ProtractorHarnessEnvironment(protractorElement(by.css('body')));
+  static loader(queryFn?: (selector: string, root: ElementFinder) => ElementArrayFinder):


We probably want to add a config object rather than passing the query impl directly in case we want to add more options in the future. I think putting it in a config also helps de-emphasize the custom querySelector impl.

jelbourn · 2020-01-16T00:35:07Z

src/cdk/testing/tests/testbed.spec.ts

@@ -12,6 +12,7 @@ import {MainComponentHarness} from './harnesses/main-component-harness';
 import {SubComponentHarness, SubComponentSpecialHarness} from './harnesses/sub-component-harness';
 import {TestComponentsModule} from './test-components-module';
 import {TestMainComponent} from './test-main-component';
+import {querySelectorAll as piercingQuerySelectorAll} from 'kagekiri';


Would a querySelector that cuts through the shadows be a luminousQuerySelectorAll?
_{that's a joke}

devversion · 2020-01-28T20:59:26Z

src/cdk/testing/tests/protractor.e2e.spec.ts

-    (selector: string, root: ElementFinder) => root.all(by.js(function(this: any) {
-      piercingQuerySelectorAll(selector, this);
-    }));
+// TODO: is there some better way to do this?


I think it's the right way to do it that way. Kagekiri can never provide types that always reserve the global kagekiri namespace. It always depends on whether the UMD bundles have been loaded or not.

I think this boilerplate is reasonable (declaring a const that needs to be named kagekiri globally). In an ideal world, it would just refer to an official type of kagekiri for type safety.

It would be also possible to avoid the boilerplate by just passing in a string to by.js. That is less type-safe, but given that we don't have official kagekiri types anyway, it seems to be unsafe anyway. I still prefer the current way though.

mmalerba · 2020-01-29T16:52:44Z

This is ready for review now. Added unit and e2e tests that demonstrate using a custom queryFn to support shadow DOM components

devversion · 2020-01-30T08:55:41Z

src/cdk/testing/kagekiri.d.ts

+
+declare module 'kagekiri' {
+  export function querySelectorAll(selector: string, root: Element): NodeListOf<Element>;
+}


This file is only used for the tests of cdk/testing right? Should we make that clear in a comment, or in the file name?

I'm not sure we have a file name convention for this, .spec.d.ts? I added a comment though

👍 for a comment. Regarding convention: My concern right now is that this file will end up in the release output since it matches the .ts glob. IMO it should be just denoted with .spec so that it automatically is filtered out, and it's obvious (without viewing the file) that it's spec only.

you mean it will get included because the bazel rule globs on *.ts? I think even renaming it to kagekiri.spec.d.ts won't fix it, since that won't match the exclude glob *.spec.ts. Should I rename it and update the bazel globs in this package?

Oh right. I think we can manually exclude the file in the exclude glob, or we change it to .spec.d.ts and update the glob to handle it. Up to you what way you choose 😄

Ok, I called it .spec.d.ts, we now officially have a precedent if we ever need to do this again

src/e2e-app/devserver-configure.js

src/cdk/testing/tests/protractor.e2e.spec.ts

jelbourn

LGTM

devversion

Looks pretty good to me! I replied on the kagekiri.d.ts file again, but I think in any case, this should be good to go.

jelbourn · 2020-02-06T16:45:48Z

@mmalerba needs rebase

This allows the user to drop in a shadow-piercing version of `querySelectorAll` to handle components that use `ViewEncapsulation.ShadowDom`

angular-automatic-lock-bot · 2020-04-09T00:02:14Z

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.}

mmalerba requested review from jelbourn and devversion January 15, 2020 16:22

mmalerba assigned jelbourn and devversion Jan 15, 2020

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 15, 2020

mmalerba force-pushed the shadow branch from ca7bc17 to 921d7dc Compare January 15, 2020 19:52

crisbeto approved these changes Jan 15, 2020

View reviewed changes

jelbourn reviewed Jan 16, 2020

View reviewed changes

mmalerba force-pushed the shadow branch 2 times, most recently from 7c586c3 to 69ca864 Compare January 17, 2020 00:34

mmalerba force-pushed the shadow branch 4 times, most recently from b664c3b to 7d83aca Compare January 28, 2020 20:16

devversion reviewed Jan 28, 2020

View reviewed changes

mmalerba marked this pull request as ready for review January 29, 2020 16:51

mmalerba requested a review from a team as a code owner January 29, 2020 16:51

devversion approved these changes Jan 30, 2020

View reviewed changes

jelbourn approved these changes Jan 30, 2020

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release labels Jan 30, 2020

devversion approved these changes Jan 31, 2020

View reviewed changes

mmalerba removed the action: merge The PR is ready for merge by the caretaker label Jan 31, 2020

mmalerba force-pushed the shadow branch 2 times, most recently from 6e0e525 to 7f52b4d Compare January 31, 2020 20:31

mmalerba added the action: merge The PR is ready for merge by the caretaker label Jan 31, 2020

mmalerba force-pushed the shadow branch from 7f52b4d to f3fdd80 Compare February 6, 2020 19:03

mmalerba added 12 commits March 4, 2020 12:22

feat(cdk/testing): Allow custom querySelectorAll method

22d03b2

This allows the user to drop in a shadow-piercing version of `querySelectorAll` to handle components that use `ViewEncapsulation.ShadowDom`

fix lint and tests

a61a4fd

exclude shadow dom tests on browsers that don't support it

1f085e6

WIP: trying to fix import paths for gulp

dffaf23

stop using ng-content

a2a27c3

WIP: attempt to get protractor working

8062fe7

make protractor tests work

965a9a9

address comments

db04610

address feedback

c825190

fix lint and api check

4e211e5

change lint to allow long urls

9514f29

exclude kagekiri declarations from release

7cf853a

mmalerba force-pushed the shadow branch from f3fdd80 to 7cf853a Compare March 4, 2020 20:22

mmalerba added 2 commits March 5, 2020 12:49

expand type of queryFn

7d2a814

remove use of + selector

1dba1d5

mmalerba merged commit 40ae1b1 into angular:master Mar 9, 2020

mmalerba deleted the shadow branch March 9, 2020 22:41

snyk-bot mentioned this pull request Mar 28, 2020

[Snyk] Upgrade: @angular/cdk, @angular/material, @angular/material-moment-adapter lingounet/testage#19

Merged

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 9, 2020

feat(cdk/testing): Allow custom querySelectorAll method #18178

feat(cdk/testing): Allow custom querySelectorAll method #18178

Uh oh!

Conversation

mmalerba commented Jan 15, 2020

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmalerba commented Jan 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

devversion left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Feb 6, 2020

Uh oh!

angular-automatic-lock-bot bot commented Apr 9, 2020

Uh oh!

Uh oh!

feat(cdk/testing): Allow custom `querySelectorAll` method #18178

feat(cdk/testing): Allow custom `querySelectorAll` method #18178