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

Fix renderRows when using in a testing environment #4214

Closed

Conversation

Nokel81
Copy link
Contributor

@Nokel81 Nokel81 commented Oct 18, 2022

Signed-off-by: Sebastian Malton sebastian@malton.name

Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81
Copy link
Contributor Author

Nokel81 commented Oct 18, 2022

I would like to include some form of unit test with this but all the tests I see in the repo are using playwright which loads a full browser instead of using something like @testing-library/react.

I ran into this while writing some tests for an application.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Oct 19, 2022

Another option could be to utilize https://developer.mozilla.org/en-US/docs/Web/API/Element/replaceChildren but I would need to check if that works

@jerch
Copy link
Member

jerch commented Oct 19, 2022

From MDN:

Setting innerText on a node removes all of the node's children and replaces them with a single text node with the given string value.

Imho the question is - why is this even an issue for you? What would replaceChildren() do differently here?
(And no, we can/should not use innerHTML ...)

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

We cannot use innerHTML as we need to be trusted types compliant. This is a very common thing to do so the testing framework should implement it.

@Tyriar Tyriar closed this Oct 19, 2022
@Nokel81
Copy link
Contributor Author

Nokel81 commented Oct 19, 2022

What do you mean by "types compliant"?

But since this does work in real browsers I am starting to suspect that @testing-library/react is at fault for not actually clearing the DOM correctly.

@Tyriar
Copy link
Member

Tyriar commented Oct 19, 2022

@Nokel81 it's a security feature that allows locking down dangerous APIs such as innerHTML which can be easily exploited https://www.w3.org/TR/2022/WD-trusted-types-20220927/

@Nokel81
Copy link
Contributor Author

Nokel81 commented Oct 19, 2022

@Tyriar Would you accept a PR that uses replaceChildren instead? I cannot find anything within the "trusted types" document you shared about that?

@Tyriar
Copy link
Member

Tyriar commented Oct 19, 2022

@Nokel81 yeah that's fine since it should be just as fast I believe

@Nokel81
Copy link
Contributor Author

Nokel81 commented Oct 19, 2022

Okay cool thanks will do then

@Tyriar
Copy link
Member

Tyriar commented Oct 19, 2022

I'd also recommend reporting an issue to support innerText = '' to @testing-library/react if that's the problem package here

@Nokel81
Copy link
Contributor Author

Nokel81 commented Oct 19, 2022

Yes I have already testing-library/react-testing-library#1146, though looking through the issue boards it might actually be an issue with jsdom itself jsdom/jsdom#1245

@Nokel81
Copy link
Contributor Author

Nokel81 commented Oct 19, 2022

Done: #4217

@Nokel81 Nokel81 deleted the fix-dom-renderer-jsdom-testing branch October 19, 2022 14:09
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.

None yet

3 participants