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

Drop old node versions #207

Merged
merged 5 commits into from
Jun 24, 2024
Merged

Drop old node versions #207

merged 5 commits into from
Jun 24, 2024

Conversation

wisko
Copy link
Contributor

@wisko wisko commented Jun 11, 2024

  • Bumped dependencies and removed support for old Node version
  • Fix assertion log message
  • Implemented replaceChildren method and wrote some tests
  • Merged the two GitHub Actions workflows

TODO:

  • If the GitHub Actions changes are okay, we need to change the settings to require PRs with the correct workflow names (I don't have access to do this)

@wisko wisko requested a review from a team as a code owner June 11, 2024 11:32
lib/Element.js Outdated
throw new DOMException("Failed to execute 'replaceChild' on 'Node': parameter 2 is not of type 'Node'.");
replaceChildren(newChildren) {
while (this.lastChild) {
this.removeChild(this.lastChild);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is an issue but removeChild / appendChild methods emits _insert events. Might result in an active mutation observer triggering multiple times during the execution of this method, rather than once after. Is this expected behavior? Otherwise might be proper to use cheerio internally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aaah, no, I just moved an implementation that we had in flamingo into tallahassee so that I could remove our local override of the class. I am not super familiar with hos these things are done in tallahassee, but I will look over the implementation!

@@ -40,8 +40,8 @@ module.exports = class WebPage {
method: "GET",
headers: requestHeaders,
});
assert.equal(resp.status, statusCode, `Unexepected status code. Expected: ${statusCode}. Actual: ${resp.statusCode}`);
assert(resp.headers.get("content-type").match(/text\/html/i), `Unexepected content type. Expected: text/html. Actual: ${resp.headers["content-type"]}`);
assert.equal(resp.status, statusCode, `Unexepected status code. Expected: ${statusCode}. Actual: ${resp.status} (GET ${uri})`);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@jonaswalden jonaswalden left a comment

Choose a reason for hiding this comment

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

👍

@jonaswalden jonaswalden merged commit bf98ee7 into master Jun 24, 2024
3 checks passed
@jonaswalden jonaswalden deleted the feat/drop-old-node-versions branch June 24, 2024 08:10
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.

2 participants