Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(queryconfig): Allows configuring a different element query mechanism #1054

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

MatthiasKainer
Copy link

@MatthiasKainer MatthiasKainer commented Oct 13, 2021

Commit opens the possibility to configure the library to use more sophisticated/different queries on elements, for instance enabling shadow dom specific queries via libraries like https://github.com/Georgegriff/query-selector-shadow-dom

What:

Provides the ability to specify a different element query mechanism other than querySelector and querySelectorAll.

An example of the usage with Georgegriff/query-selector-shadow-dom looks like this:

import { querySelectorAllDeep, querySelectorDeep } from 'query-selector-shadow-dom';
configure({
    queryElement: querySelectorDeep,
    queryAllElements: querySelectorAllDeep,
})

After this line, a query will also drill into the shadow dom of elements on the page

Why:

In our project, we are using a design system build with web components (via stenciljs), and different projects in different frameworks (lit, react, vue) want to use this library. However, due to the issues already highlighted in #742 #413 and in a wider part also #999 I tried to come up with an approach that would allow extension for such scenarios without having to introduce modification in the core library.

How:

I implemented the changes in a way they are non-breaking. In the types/config.d.ts I added the fields queryElement and queryAllElements as optional so that any existing typings in projects won't break. In the config.ts I added it as required fields to the InternalConfig to ensure that it is always available. I set them to querySelector and querySelectorAll to make sure it's doing the same as before.

Then, in the files that were using querySelector and querySelectorAll I replaced the calls with the method exposed by the config.

The next step would probably be to make the prettyDOM implementation configurable as well, ie to allow fixing of #999

Ideally, I was thinking about introducing a field "domHandling" that would contain all that, but it felt overblown for now. Open for discussion I guess.

Checklist:

  • Documentation added to the
    docs site
  • Tests
    I added one test, as I felt it wouldn't provide a lot of value given the core functionality is the same. I'd expect the providers of extension to test it. Would you agree?
  • TypeScript definitions updated
  • Ready to be merged

Looking forward to your feedback!

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 13, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 622981f:

Sandbox Source
react-testing-library-examples Configuration


configure({
queryAllElements: (element, query) =>
element.querySelectorAll(`#other ${query}`),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you provide another example that can't be implement with just using a different container? In the PR you refer to Shadow DOM related issues but no concrete example or test is given that explains how this API adresses these issues.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I will try to come up with something; will have to learn how to create a JSDOM fragment with shadow dom for that - if you have an idea pls let me know, I haven't really worked with their API yet.

Is it okay if won't be a very generic solution, ie just [...element.querySelectorAll(query), ...element.shadowRoot.querySelectorAll(query)] to go one layer deep, or should I actually add a new test dependency to ie Georgegriff/query-selector-shadow-dom to show the complete scenario? Any preferences?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I will try to come up with something; will have to learn how to create a JSDOM fragment with shadow dom for that - if you have an idea pls let me know, I haven't really worked with their API yet.

Me neither. Shadow DOM is not really something most maintainers work with so we rely on community support.

or should I actually add a new test dependency to ie Georgegriff/query-selector-shadow-dom to show the complete scenario? Any preferences?

Showing hypothetical integration is ok. Though I'm still skeptical about the generic solution. That can also be implemented with a wrapper that just queries once with container: element and a second time with container: element.shadowRoot.

How is the integration currently working?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Showing hypothetical integration is ok. Though I'm still skeptical about the generic solution. That can also be implemented with a wrapper that just queries once with container: element and a second time with container: element.shadowRoot.

That would only work for the first layer. A shadow root can contain another shadow root which can contain another shadow root on so on, and you will have to do a few checks to use a javascript API to traverse the tree without any errors. I'm totally okay (and would be super happy actually!) if you'd think it would be okay adding support for it natively - it would possibly mean adding another dependency though or taking ownership of quite a bit of code.

How is the integration currently working?

The only working integration I know of is testing-library__dom from @Westbrook. We are using that for lit-based components that we test with the testing library. It's basically taking the generated dom.js from dom-testing-library, replacing the calls to "querySelectorAll" with "querySelectorAllWithShadow" in the source code, and poly-filling it. Please correct me if I'm wrong, @Westbrook!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted the test to use a custom element with shadow dom. The implementation of the selector is naive, and will not really scale/work in a real-world scenario, but I hope it's more clear now. Let me know what you think!

Note I'm down with fever today, so my answers will be delayed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eps1lon did the test clarify the shadow-dom use case a little better? How shall we continue? You were wondering about the generic solution, should I create an alternative PR thats using an external library, or shall we wait to see if the generic approach also helps @alexkrolick with cypress?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have time to rebuild cypress-testing-lib using this methodology

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for the info. I'm already prepping a second PR then with a less generic approach, then we can discuss the advantages/disadvantages of both.

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #1054 (673c5e9) into main (54bfa48) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 673c5e9 differs from pull request most recent head 622981f. Consider uploading reports for the commit 622981f to get more accurate results

@@            Coverage Diff            @@
##              main     #1054   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        25    +1     
  Lines          985       955   -30     
  Branches       321       312    -9     
=========================================
- Hits           985       955   -30     
Flag Coverage Δ
node-12 100.00% <100.00%> (ø)
node-14 100.00% <100.00%> (ø)
node-16 ?
node-16.9.1 100.00% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/queries/display-value.ts 100.00% <ø> (ø)
src/queries/role.js 100.00% <ø> (ø)
src/queries/text.ts 100.00% <ø> (ø)
src/queries/title.ts 100.00% <ø> (ø)
src/query-helpers.ts 100.00% <ø> (ø)
src/config.ts 100.00% <100.00%> (ø)
src/label-helpers.ts 100.00% <100.00%> (ø)
src/queries/label-text.ts 100.00% <100.00%> (ø)
src/helpers.js 100.00% <0.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54bfa48...622981f. Read the comment docs.

@alexkrolick
Copy link
Collaborator

Have not reviewed thoroughly, but would this change allow something like the Cypress wrapper to put cy.get() as the querySelectorAll implementation, allowing hooking into the base retry methods?

@MatthiasKainer
Copy link
Author

MatthiasKainer commented Oct 14, 2021

Have not reviewed thoroughly, but would this change allow something like the Cypress wrapper to put cy.get() as the querySelectorAll implementation, allowing hooking into the base retry methods?

I don't know the cypress well enough to answer that. The contract (types) of the function is basically the same as querySelector[All], so if the function returns an HTML Element Array (it seems it does) then it works I guess. I'm not entirely sure I fully understand the API - especially if it is async or sync. If it's async then there might be more issues, this interface is set up to work synchronous only.

From skimming the documentation it looks as if the cy.get method is executed globally, whereas in the testing-library the querySelectorAll method is executed on a passed element. You seem to be able to chain html elements, so if it's similar to jquery as claimed, then probably you could create a working implementation with something like

configure({
    queryAllElements:(element, query) => cy.get(element).get(query),
})

But as said, I don't know cypress well enough to know if this works. Maybe try it out by checking out this PR, run npm run setup && npm run build && npm install . which would install this package version locally. Then you can run it with your cypress suite.

Would love to hear feedback and get ideas how to improve this to make it work for your scenario as well.

/edit - added npm command for building

@MatthiasKainer
Copy link
Author

@eps1lon I was thinking about this quite a bit, and looking at the test adjustments needed by directly including a query mechanism for shadow dom let me to the conclusion that it might be a breaking change for many (see https://github.com/testing-library/dom-testing-library/pull/1069/files#diff-fd31e410b487b45eac250e578eaa42419a3321f9664ce1db42f08fd8cc21aefb for an example)

I would thus change this PR to no longer be a draft and propose the merge, and from there continue with the following direction:

  • For the next major release, switch the element's query method to deep queries, with the option to provide an option to gracefully switch back to the old query.
  • For the release after that, remove the fallback option and the indirection.

I will remove the draft status now, if this approach is okay for you and the PR can be merged, I will close #1069

@MatthiasKainer MatthiasKainer changed the title Draft: feat(queryconfig): Allows configuring a different element query mechanism feat(queryconfig): Allows configuring a different element query mechanism Nov 16, 2021
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation is missing as well. This should be added beforehand by illustrating the problem it's trying to solve.

src/config.ts Outdated Show resolved Hide resolved
types/__tests__/type-tests.ts Outdated Show resolved Hide resolved
src/__node_tests__/index.js Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
MatthiasKainer added a commit to MatthiasKainer/testing-library-docs that referenced this pull request Nov 16, 2021
Provides the ability to specify a different element query mechanism other than querySelector and querySelectorAll.

An example of the usage with Georgegriff/query-selector-shadow-dom looks like this:

import { querySelectorAllDeep, querySelectorDeep } from 'query-selector-shadow-dom';
configure({
    queryElement: querySelectorDeep,
    queryAllElements: querySelectorAllDeep,
})
After this line, a query will also drill into the shadow dom of elements on the page

Why:

In our project, we are using a design system build with web components
(via stenciljs), and different projects in different frameworks (lit,
react, vue) want to use this library. The corresponding pr can be found
here:
testing-library/dom-testing-library#1054
@MatthiasKainer
Copy link
Author

MatthiasKainer commented Nov 16, 2021

Created docs here: testing-library/testing-library-docs#964

Broke the git history, will clean that up next
/edit: fixed

…nism

Commit opens the possibility to configure
the api to use more sophisticated queries on
elements, for instance enabling shadow dom
specific queries via libraries like
https://github.com/Georgegriff/query-selector-shadow-dom
By adding ":scope >" to the css selector we avoid
returning duplicate results if anybody wants to
extend this test in the future.
@MatthiasKainer
Copy link
Author

@eps1lon do you need anything else to move this forward?

@surfer19
Copy link

surfer19 commented Dec 2, 2021

@MatthiasKainer @eps1lon
hello everyone, is it possible to provide this feature also to @react-testing-library? Does @react-testing somehow derives functionalities from main @dom-testing or is it 100% separated?
I'm having same scenario that I need to resolve.

I would expect querying elements behind shadow dom will be more and more common for all kind of projects.
At example very common scenario is to have design system components (based on lit-element) consumed by other app (React,..).

Thank you!

@MatthiasKainer
Copy link
Author

@surfer19 Yes, react-testing-tools is only a wrapper around the dom-testing-tools (see https://github.com/testing-library/react-testing-library/blob/main/src/pure.js#L119).

Integrating a react app with react-testing-tools with a stenciljs design system (with shadow dom) is the original usecase for which I created the PR. I tested this scenario with this addition and it works.

@surfer19
Copy link

surfer19 commented Dec 3, 2021

@MatthiasKainer cool, does it support nested tree of the web components?
Like:

#shadow-root
 ...
   #shadow-root
      ....
         #shadow-root

@MatthiasKainer
Copy link
Author

@surfer19 jop, if you use query-selector-shadow-dom as shown in the description it will reach into arbitrary deep shadow doms.

If this request is merged I would also use the opportunity to create a pr to change the default query mechanism to shadow-dom, so you will no longer be required to use another library; But as this is a breaking change (ie the order of elements might be changed, see https://github.com/testing-library/dom-testing-library/pull/1069/files#diff-fd31e410b487b45eac250e578eaa42419a3321f9664ce1db42f08fd8cc21aefbR134) and performance might be worse, my proposal after creating both PRs was to go for a stepwise approach. Does that make sense?

@MatthiasKainer
Copy link
Author

Are there any updates? Is there something else I should add?

@surfer19
Copy link

surfer19 commented Feb 2, 2022

@eps1lon @MatthiasKainer
Any updates? Based on community opinion, it seems to be something that might be worth to finish.
#413 (comment)
#413 (comment)

@MatthiasKainer
Copy link
Author

I just merged the latest changes main, and the build fails. I will fix this and try again

@MatthiasKainer
Copy link
Author

okay, I tried to follow codesandbox to the letter, including installing the same node version

❯ git clone https://github.com/testing-library/dom-testing-library.git
Cloning into 'dom-testing-library'...
remote: Enumerating objects: 3523, done.
remote: Counting objects: 100% (511/511), done.
remote: Compressing objects: 100% (329/329), done.
remote: Total 3523 (delta 294), reused 303 (delta 167), pack-reused 3012
Receiving objects: 100% (3523/3523), 1.55 MiB | 3.97 MiB/s, done.
Resolving deltas: 100% (2467/2467), done.
❯ cd dom-testing-library
❯ git fetch --force origin pull/1054/head:remotes/origin/pull/1054
remote: Enumerating objects: 46, done.
remote: Counting objects: 100% (46/46), done.
remote: Compressing objects: 100% (12/12), done.
remote: Total 46 (delta 33), reused 46 (delta 33), pack-reused 0
Unpacking objects: 100% (46/46), 8.70 KiB | 387.00 KiB/s, done.
From https://github.com/testing-library/dom-testing-library
 * [new ref]         refs/pull/1054/head -> origin/pull/1054
❯ git checkout -qf remotes/origin/pull/1054
❯ node -v
v16.13.0
❯ nvm install v12.22.7
Downloading and installing node v12.22.7...
Downloading https://nodejs.org/dist/v12.22.7/node-v12.22.7-linux-x64.tar.xz...
################################################################################################################################ 100.0%
Computing checksum with sha256sum
Checksums matched!
Now using node v12.22.7 (npm v6.14.15)
❯ node -v
v12.22.7
❯ yarn -v
1.22.17
❯ yarn install
yarn install v1.22.17
info No lockfile found.
[1/5] Validating package.json...
[2/5] Resolving packages...
warning @testing-library/jest-dom > css > source-map-resolve@0.6.0: See https://github.com/lydell/source-map-resolve#deprecated
warning kcd-scripts > cpy > globby > fast-glob > micromatch > snapdragon > source-map-resolve@0.5.3: See https://github.com/lydell/source-map-resolve#deprecated
warning kcd-scripts > rollup-plugin-node-builtins > browserify-fs > level-filesystem > level-sublevel > xtend > object-keys@0.2.0: Please update to the latest object-keys
warning kcd-scripts > cpy > globby > fast-glob > micromatch > snapdragon > source-map-resolve > resolve-url@0.2.1: https://github.com/lydell/resolve-url#deprecated
warning kcd-scripts > cpy > globby > fast-glob > micromatch > snapdragon > source-map-resolve > source-map-url@0.4.1: See https://github.com/lydell/source-map-url#deprecated
warning kcd-scripts > cpy > globby > fast-glob > micromatch > snapdragon > source-map-resolve > urix@0.1.0: Please see https://github.com/lydell/urix#deprecated
[3/5] Fetching packages...
[4/5] Linking dependencies...
[5/5] Building fresh packages...
success Saved lockfile.
Done in 37.14s.
❯ yarn run build
yarn run v1.22.17
$ kcd-scripts build  --no-ts-defs --ignore "**/__tests__/**,**/__node_tests__/**,**/__mocks__/**" && kcd-scripts build --no-ts-defs --bundle --no-clean
Successfully compiled 29 files with Babel (969ms).
[cjs]
[cjs] /home/mkainer/projects/private/testing-tools/branch/dom-testing-library/src/index.js → dist/@testing-library/dom.cjs.js...
[umd.min]
[umd.min] /home/mkainer/projects/private/testing-tools/branch/dom-testing-library/src/index.js → dist/@testing-library/dom.umd.min.js...
[umd]
[umd] /home/mkainer/projects/private/testing-tools/branch/dom-testing-library/src/index.js → dist/@testing-library/dom.umd.js...
[esm]
[esm] /home/mkainer/projects/private/testing-tools/branch/dom-testing-library/src/index.js → dist/@testing-library/dom.esm.js...
[esm] (!) Plugin replace: @rollup/plugin-replace: 'preventAssignment' currently defaults to false. It is recommended to set this option to `true`, as the next major version will default this option to `true`.
[esm] created dist/@testing-library/dom.esm.js in 2.1s
[esm] cross-env BUILD_ROLLUP=true BUILD_FORMAT=esm BUILD_MINIFY=false NODE_ENV=development BUILD_PREACT=false BUILD_SIZE_SNAPSHOT=undefined BUILD_NODE=false BUILD_REACT_NATIVE=false rollup --config exited with code 0
[cjs] (!) Plugin replace: @rollup/plugin-replace: 'preventAssignment' currently defaults to false. It is recommended to set this option to `true`, as the next major version will default this option to `true`.
[cjs] created dist/@testing-library/dom.cjs.js in 2.2s
[cjs] cross-env BUILD_ROLLUP=true BUILD_FORMAT=cjs BUILD_MINIFY=false NODE_ENV=development BUILD_PREACT=false BUILD_SIZE_SNAPSHOT=undefined BUILD_NODE=false BUILD_REACT_NATIVE=false rollup --config exited with code 0
[umd] (!) Plugin replace: @rollup/plugin-replace: 'preventAssignment' currently defaults to false. It is recommended to set this option to `true`, as the next major version will default this option to `true`.
[umd] (!) Circular dependency
[umd] node_modules/dom-accessibility-api/dist/util.mjs -> node_modules/dom-accessibility-api/dist/getRole.mjs -> node_modules/dom-accessibility-api/dist/util.mjs
[umd] created dist/@testing-library/dom.umd.js in 6.5s
[umd] cross-env BUILD_ROLLUP=true BUILD_FORMAT=umd BUILD_MINIFY=false NODE_ENV=development BUILD_PREACT=false BUILD_SIZE_SNAPSHOT=undefined BUILD_NODE=false BUILD_REACT_NATIVE=false rollup --config --sourcemap exited with code 0
[umd.min] (!) Plugin replace: @rollup/plugin-replace: 'preventAssignment' currently defaults to false. It is recommended to set this option to `true`, as the next major version will default this option to `true`.
[umd.min] (!) Circular dependency
[umd.min] node_modules/dom-accessibility-api/dist/util.mjs -> node_modules/dom-accessibility-api/dist/getRole.mjs -> node_modules/dom-accessibility-api/dist/util.mjs
[umd.min] created dist/@testing-library/dom.umd.min.js in 8.3s
[umd.min] cross-env BUILD_ROLLUP=true BUILD_FORMAT=umd BUILD_MINIFY=true NODE_ENV=production BUILD_PREACT=false BUILD_SIZE_SNAPSHOT=undefined BUILD_NODE=false BUILD_REACT_NATIVE=false rollup --config --sourcemap exited with code 0
Done in 10.68s.

Finished successfully. Can someone with permissions retrigger the codesandbox build?

@MatthiasKainer
Copy link
Author

@eps1lon any updates on how we can proceed here?

@MatthiasKainer
Copy link
Author

@eps1lon everything should be resolved - can we merge this eventually?

@MatthiasKainer
Copy link
Author

If there's something missing I'd be super happy to tackle it. Pls let me know how to proceed

@manuscriptmastr
Copy link

manuscriptmastr commented May 16, 2022

@eps1lon @alexkrolick @MatthiasKainer just wanted to bump this PR ✨ I'm really hoping this gets through soon so I can recommend Testing Library for testing web components at my company! (Been giving a web components workshop this spring.) This is such a huuuuge win for Testing Library, I'd hate to see it get stale.

@CreativeTechGuy
Copy link

While I really support this change, one thing I see missing is the handling of slots. Simply querying for all elements won't work correctly as the slotted content won't be correctly attached. I don't think we can solve this simply by changing the querySelector method but would need special handling for each Testing Library query.

For example, textContent of a slot will be whitespace. You'd first need to do slot.assignedNodes({ flatten: true }) to get the contents of the slot and then traverse those elements.

I don't see why this cannot be a flag in the core library, something like traverseShadowDOM: true in the config which would then swap out the handling of each query internally to handle these things. I have a strong need for this and would be happy to contribute if we can get some signal that this would be merged if solved.

@alexkrolick
Copy link
Collaborator

@CreativeTechGuy thanks for pointing this out. I've noticed that libraries like Playwright and Cypress have ShadowDOM-piercing support. Maybe we should look into what they do.

@MatthiasKainer
Copy link
Author

MatthiasKainer commented Jun 24, 2022

The library i mentioned above is often used with playwright to enable deep queries - https://github.com/Georgegriff/query-selector-shadow-dom

I also had a PR that was using that directly, and closed it because lack of downwards compatibility

@manuscriptmastr
Copy link

@alexkrolick @MatthiasKainer Any updates on this PR?

@MatthiasKainer
Copy link
Author

@alexkrolick @MatthiasKainer Any updates on this PR?

I don't know what else I could do. From my side this PR could be merged. As it only creates the possibility to extend queries, it shouldn't have any impact to existing installations. As you can use it with a library like https://github.com/Georgegriff/query-selector-shadow-dom you can use it to enable deep queries into shadow Dom's.

If there's anything I should add it would be great if someone could let me know, otherwise I'm just waiting for someone to accept it

@odanado
Copy link

odanado commented Jan 12, 2024

@alexkrolick Any updates on this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants