-
Notifications
You must be signed in to change notification settings - Fork 467
feat(queryconfig): Allows configuring a different element query mechanism #1054
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
Conversation
|
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:
|
src/__node_tests__/index.js
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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?
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
@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
There was a problem hiding this 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.
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
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.
25a3380 to
df77ec3
Compare
@eps1lon do you need anything else to move this forward?
@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!
@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
commented
Dec 3, 2021
@MatthiasKainer cool, does it support nested tree of the web components?
Like:
#shadow-root
...
#shadow-root
....
#shadow-root
@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?
Are there any updates? Is there something else I should add?
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)
I just merged the latest changes main, and the build fails. I will fix this and try again
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?
@eps1lon any updates on how we can proceed here?
@eps1lon everything should be resolved - can we merge this eventually?
If there's something missing I'd be super happy to tackle it. Pls let me know how to proceed
@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
commented
May 17, 2022
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.
@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.
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
commented
Aug 22, 2022
@alexkrolick @MatthiasKainer Any updates on this PR?
@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
commented
Jan 12, 2024
@alexkrolick Any updates on this PR?
Uh oh!
There was an error while loading. Please reload this page.
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
querySelectorandquerySelectorAll.An example of the usage with Georgegriff/query-selector-shadow-dom looks like this:
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.tsI added the fieldsqueryElementandqueryAllElementsas optional so that any existing typings in projects won't break. In theconfig.tsI added it as required fields to theInternalConfigto ensure that it is always available. I set them toquerySelectorandquerySelectorAllto make sure it's doing the same as before.Then, in the files that were using
querySelectorandquerySelectorAllI 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:
docs site
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?
Looking forward to your feedback!