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.

Sign up for GitHub

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

Jump to bottom

Test on all minimum supported Node.js versions #1170

Merged
merged 14 commits into from
Jul 16, 2021
Merged

Test on all minimum supported Node.js versions #1170

merged 14 commits into from
Jul 16, 2021

Conversation

LinusU
Copy link
Member

@LinusU LinusU commented May 25, 2021

What is the purpose of this pull request?

  • Other, please explain: CI update for ESM

What changes did you make? (provide an overview)

When switching to ESM in #1141 we are introducing three new minimum Node.js versions that we support:

  • 12.20.0
  • 14.13.1
  • 16.0.0

In order to not accidentally break any of those versions I propose that we test on them all.

This was previously sort of done with the minimum-node-version but it only supports a single version, so would thus only run on 12.20.0.

Which issue (if any) does this pull request address?

n/a

Is there anything you'd like reviewers to know?

n/a

ping @jimmywarting: I didn't push this directly to the esm branch since this might something we want to discuss first?

@LinusU
Copy link
Member Author

LinusU commented May 25, 2021 •
edited
Loading

Seems like this uncovered some issues that needs to be fixed:

  • Node.js 14.13.1 doesn't seem to have the fix checked for here:

    if (process.version < 'v14') {

    should handle network-error in chunked response async iterator:
    AssertionError: expected promise to be rejected with 'Error' but it was fulfilled with [ Array(2) ]

  • Node.js 16.x seems to have changed error message:

    AssertionError: expected 'Invalid response body while trying to fetch http://localhost:34163/error/premature: aborted' to match /Premature close|The operation was aborted/

  • Node.js 16.x seems to have changed something with highWaterMark

    should not timeout on cloning response without consuming one of the streams when the second packet size is less than default highWaterMark:
    AssertionError: expected promise not to timeout but it timed out

    should not timeout on cloning response without consuming one of the streams when the second packet size is less than custom highWaterMark:
    AssertionError: expected promise not to timeout but it timed out

    should not timeout on cloning response without consuming one of the streams when the response size is double the custom large highWaterMark - 1:
    AssertionError: expected promise not to timeout but it timed out

edit: I'm not sure about the last one, would love some input!

@jimmywarting
Copy link
Collaborator

edit: I'm not sure about the last one, would love some input!

I don't know either, but like the looks of this PR

@LinusU
Copy link
Member Author

LinusU commented Jun 16, 2021

@xxczaki it seems like you originally did the high watermark implementation (#671). Would you be able to take a quick look at the failure on Node.js 16 and see if you know what's wrong? Thanks!

@jimmywarting
Copy link
Collaborator

jimmywarting commented Jun 29, 2021 •
edited
Loading

Don't know where all the old maintainers/contributors have gone... vacation maybe?
Speculating wether or not to just disable the test for the response cloning and expect things to fail again if you clone the response without using the other stream

Want to push for the v3 and the ESM PR out for stable release soonish

Also speculating if WeakRef/Finalizers could help to clean up un-used pipe/stream if it's necessary for one of the 2nd stream to finish. It seems to be the major issue for developers that have that problem. That it won't finish the 2nd stream if the 1st one is buffered/filled and forgotten/unused. It was also the reason why setting a none standard highWatherMark was added... Ideally i would like to get rid of the highWatherMark option if we can to stay closer to the spec

No commits has happen for 2 months and it feels like we have goten stuck somewhere, where everybody expect someone else to fix/review/accept things or no longer have time

@jimmywarting
Copy link
Collaborator

jimmywarting commented Jun 29, 2021 •
edited
Loading

looking back at old PR's it seems like @davesidious has a solution/pr that i have overlooked that solves highWaterMark #1162 ?

Don't know if it's related to node v16 or just some old mistake we have done in v3

Edit: tried 1162 on my machine, didn't solve v16

@LinusU
Copy link
Member Author

LinusU commented Jul 15, 2021 •
edited
Loading

@jimmywarting 👍

hmmmm, this is strange:

SyntaxError[ @/home/runner/work/node-fetch/node-fetch/test/external-encoding.js ]: Unexpected token '.'

🤔

jimmywarting
jimmywarting reviewed Jul 15, 2021
View reviewed changes
test/main.js Outdated Show resolved Hide resolved
@jimmywarting
Copy link
Collaborator

Found what was wrong with fetch-blob and fixed it. it's released now.

Dose the changes looks good to be merged @LinusU ? I'm rdy to merge

@jimmywarting jimmywarting merged commit 44c899f into esm Jul 16, 2021
@jimmywarting jimmywarting deleted the linusu-esm-ci branch July 16, 2021 07:42
LinusU
LinusU commented Jul 16, 2021
View reviewed changes
test/main.js
Comment on lines +40 to +42
function isNodeLowerThan(version) {
return !~process.version.localeCompare(version, undefined, {numeric: true});
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Neat! 👍

@LinusU
Copy link
Member Author

LinusU commented Jul 16, 2021

Looks great! Thanks for landing this 🚀

jimmywarting added a commit that referenced this pull request Jul 18, 2021
@xxczaki @jimmywarting @LinusU
Require Node.js 12.20.0 and move to ESM (#1141)
b50fbc1 
* Use ESM import in runkit example file

* Update dependencies, version and transition to ESM

* Use ESM imports, add ESM-related info

* Remove rollup

* Lint TypeScript-related files

* Update dependency

* Lint & update dependency

* Lint

* Remove commonjs tests

* chore: update changelog

* Remove commonjs GitHub action

* Update funding.yml

* Update linter rules

* Lint

* Fix tsd

* Remove unnecessary types

* Simplify

* Use top-level await

* Update GitHub Actions

* Use Mocha with ESM

* Revamp

* specify what node version

* update formdata-node dep

* remove lint from example using top await

* updated name and link to formdata-polyfill

* Stop recommend form-data

* filter example - it has many duplicate variables

* Update type definitions to ESM

* Remove unused lint rule disable comment

* Remove leftover rollup and dist folder

* updated depn

* updated d.ts

* lint

* Fix breaking changes with blob v3 stream()

* revert eslint comment

* revert back to xo 0.39

Don't want to deal with all those new rules right now. will fix it later
fixed some of them...

* none TS fan trying to fix type definition

* Give me a break

* Test on all minimum supported Node.js versions (#1170)

* Test on all minimum supported Node.js versions

* Tweak Node.js workaround version range

* Handle Node.js 16 aborted error message

* fix node version string compare

Co-authored-by: Jimmy Wärting <jimmy@warting.se>

* bumped fetch-blob version

* import from dom lib

* rm unused comment

* updated required version in docs

* fixed named import

* set lowest support to 12.20.0

* comment explaining both

* rm log

Co-authored-by: Jimmy Wärting <jimmy@warting.se>
Co-authored-by: Linus Unnebäck <linus@folkdatorn.se>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimmywarting jimmywarting jimmywarting approved these changes

@tinovyatkin tinovyatkin Awaiting requested review from tinovyatkin

@tekwiz tekwiz Awaiting requested review from tekwiz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@LinusU @jimmywarting