-
Notifications
You must be signed in to change notification settings - Fork 31
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
base: main
Are you sure you want to change the base?
Conversation
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.
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
@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() |
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.
Looks like this introduces a regression in tests see https://github.com/web-std/io/actions/runs/5986735537/job/16286188008?pr=86
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.
@mcansh Any idea what this could have caused?
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.
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
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.
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.
26f3294
to
a5a297b
Compare
9c7b1db
to
126988a
Compare
0ef14d2
to
64fdbb4
Compare
* 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]>
Signed-off-by: Logan McAnsh <[email protected]>
792ae4b
to
c5f7708
Compare
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.
Copilot reviewed 15 out of 24 changed files in this pull request and generated 1 comment.
Files not reviewed (9)
- package.json: Language not supported
- packages/blob/package.json: Language not supported
- packages/fetch/.github/FUNDING.yml: Language not supported
- packages/fetch/.github/ISSUE_TEMPLATE/config.yml: Language not supported
- packages/fetch/.github/workflows/ci.yml: Language not supported
- packages/fetch/.github/workflows/commonjs.yml: Language not supported
- packages/fetch/.github/workflows/lint.yml: Language not supported
- packages/fetch/.github/workflows/types.yml: Language not supported
- packages/fetch/package.json: Language not supported
Comments suppressed due to low confidence (6)
packages/fetch/src/headers.js:141
- Verify that using 'target' in place of 'receiver' is appropriate for this call to maintain consistent behavior across header operations.
target,
packages/fetch/.github/PULL_REQUEST_TEMPLATE.md:1
- [nitpick] The removal of the pull request template may be unintended; please confirm if this change aligns with the team's workflow expectations.
<file removed>
packages/fetch/.github/ISSUE_TEMPLATE/support-or-usage.md:1
- [nitpick] The removal of the support or usage issue template should be verified to ensure it was a deliberate decision and not an accidental deletion.
<file removed>
packages/fetch/.github/ISSUE_TEMPLATE/feature-request.md:1
- [nitpick] Please confirm that the removal of the feature request issue template was intentional and that it does not disrupt the expected issue reporting process.
<file removed>
packages/fetch/.github/ISSUE_TEMPLATE/bug_report.md:1
- [nitpick] Ensure that removing the bug report issue template aligns with your team's reporting requirements and that an alternative is in place if needed.
<file removed>
.github/workflows/fetch.yml:72
- [nitpick] The Coveralls step has been removed from the workflow. Confirm that this removal is intentional and that an alternative coverage reporting solution is in place if needed.
# upload coverage only once
See @mcansh's remix-run#32
CC/ @alanshaw @Gozala