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: "ERR_INVALID_THIS" on Node 20 #86

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

Conversation

MichaelDeBoey
Copy link
Contributor

Copy link
Contributor

@Gozala Gozala left a comment

Choose a reason for hiding this comment

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

Please note that formatting and other inconsistencies in the fetch sub-package were due to it been forked from node-fetch that used particular style. I thought it would be easier to pull upstream changes this way.

That said I have not really pulled any changes from node-fetch so perhaps that decision is worth revisiting. Given you've put more time into this than myself, you should make call what makes more sense going forward. I have no objections either way

@Gozala
Copy link
Contributor

Gozala commented Aug 28, 2023

@MichaelDeBoey looks like this breaks some tests, could you please fix them before landing this ?

@@ -138,7 +138,7 @@ export default class Headers extends URLSearchParams {
validateHeaderName(name);
// @ts-ignore
return URLSearchParams.prototype[p].call(
receiver,
target,
String(name).toLowerCase()
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this introduces a regression in tests see https://github.com/web-std/io/actions/runs/5986735537/job/16286188008?pr=86

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mcansh Any idea what this could have caused?

Copy link
Contributor

@mcansh mcansh Aug 31, 2023

Choose a reason for hiding this comment

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

narrowed it down to internal version mismatches between packages so installing/building the packages weren't using the latest changes

edit: tested with pnpm and workspace:* the packages together

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 Author

Choose a reason for hiding this comment

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

That would have been fixed if #74 was already merged then

@Gozala another reason to at least include manypkg (but I would do the switch to changeset as well) 🙈

mcansh and others added 4 commits September 1, 2023 00:50
* fix: node 20

pnpm/node-fetch#1
Signed-off-by: Logan McAnsh <[email protected]>

* ci: test against more node versions

Signed-off-by: Logan McAnsh <[email protected]>

* ci: update action versions

Signed-off-by: Logan McAnsh <[email protected]>

* chore: remove nested .github dir

Signed-off-by: Logan McAnsh <[email protected]>

* ci: disable fail fast

Signed-off-by: Logan McAnsh <[email protected]>

* ci: new server per test

Signed-off-by: Logan McAnsh <[email protected]>

* test: update family agent option test

Signed-off-by: Logan McAnsh <[email protected]>

* Create polite-eggs-jam.md

* test: use ipv4

Signed-off-by: Logan McAnsh <[email protected]>

* test: use family 0

Signed-off-by: Logan McAnsh <[email protected]>

---------

Signed-off-by: Logan McAnsh <[email protected]>
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.

3 participants