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 a bug in createTextNode when the data argument is a non-string #1384

Merged
merged 3 commits into from Apr 5, 2024

Conversation

odanado
Copy link
Contributor

@odanado odanado commented Apr 5, 2024

Close #1380

Run in Chrome:

 2024-04-06 1 30 54

Run in Safari:
 2024-04-06 1 31 22

Run in Firefox:
 2024-04-06 1 33 02

@@ -1165,8 +1165,13 @@ export default class Document extends Node {
* @param [data] Text data.
* @returns Text node.
*/
public createTextNode(data?: string): Text {
return NodeFactory.createNode<Text>(this, this[PropertySymbol.ownerWindow].Text, data);
public createTextNode(data: string): Text {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In TypeScript lib.dom.d.ts, the createTextNode argument is not optional.

 2024-04-06 1 57 31

I have changed the type and implementation to mimic browser behavior.

Safari Firefox Chrome
 2024-04-06 1 50 46  2024-04-06 1 51 00  2024-04-06 1 50 53

Could you help me determine whether this change is considered a 'bug fix' or a 'breaking change'?

Copy link
Owner

Choose a reason for hiding this comment

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

I would consider this as a bug fix as it is very unlikely to affect a consumer, but of course it would be great if this fix could be added as part of a major release. I think it is fine for this case.

@odanado
Copy link
Contributor Author

odanado commented Apr 5, 2024

   FAIL  test/fetch/SyncFetch.test.ts > SyncFetch > send() > Updates cache after a failed revalidation with a "If-Modified-Since" request for a GET response with "Cache-Control" set to a "max-age".
  AssertionError: expected 'some text' to be 'some new text' // Object.is equality

I noticed that the CI tests failed after I submitted my pull request.
However, the failure appears to be related to a test that is not associated with the changes I've made. I suspect it might be a flaky test.
Could you please advise on how to proceed? Should I try rerunning the tests, or is there anything else you'd recommend?

Copy link
Owner

@capricorn86 capricorn86 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @odanado! ⭐

@capricorn86
Copy link
Owner

   FAIL  test/fetch/SyncFetch.test.ts > SyncFetch > send() > Updates cache after a failed revalidation with a "If-Modified-Since" request for a GET response with "Cache-Control" set to a "max-age".
  AssertionError: expected 'some text' to be 'some new text' // Object.is equality

I noticed that the CI tests failed after I submitted my pull request. However, the failure appears to be related to a test that is not associated with the changes I've made. I suspect it might be a flaky test. Could you please advise on how to proceed? Should I try rerunning the tests, or is there anything else you'd recommend?

It's a known problem that has to be fixed. It is related to a timing issue with the cache expire time for in Fetch.ts and SyncFetch.ts.

@capricorn86 capricorn86 merged commit 86c9166 into capricorn86:master Apr 5, 2024
3 checks passed
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.

data argument of createTextNode is not coerced into a string
2 participants