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

Is jest-polly compatible with Node 18.3+ fetch #342

Open
guyellis opened this issue Feb 16, 2023 · 8 comments · May be fixed by #344
Open

Is jest-polly compatible with Node 18.3+ fetch #342

guyellis opened this issue Feb 16, 2023 · 8 comments · May be fixed by #344

Comments

@guyellis
Copy link

From Node 18.3+ onward native fetch is available on the global object. Does jest-polly work with this?

@moltar
Copy link
Contributor

moltar commented Feb 17, 2023

I am not sure, have not tested.

Ultimately, this package uses @pollyjs/adapter-node-http.

This package does not use node-fetch in any way, other than just in tests.

@moltar
Copy link
Contributor

moltar commented Feb 17, 2023

Added Node 18 to the test matrix - tests pass! Closing the issue, but feel free to reopen it or leave your feedback here if something is not working.

@moltar moltar closed this as completed Feb 17, 2023
@guyellis
Copy link
Author

guyellis commented Feb 17, 2023

Thanks for the reply @moltar

I just looked at your Add Node 18 to tests commit and see that this is testing node-fetch under Node 18. Always good to have this coverage. Thanks for adding this.

What I'm talking about here is that fetch is now available on global from Node 18 onward. If you use nvm to manage your Node versions then this should demonstrate:

$ nvm use 18
Now using node v18.12.1 (npm v8.19.2)

$ node
Welcome to Node.js v18.12.1.
Type ".help" for more information.
> fetch
[AsyncFunction: fetch]

$ nvm use 16
Now using node v16.18.1 (npm v8.19.2)

$ node
Welcome to Node.js v16.18.1.
Type ".help" for more information.
> fetch
Uncaught ReferenceError: fetch is not defined

PollyJS have a Fetch Adapter that has the byline:

The fetch adapter wraps the global fetch method for seamless recording and replaying of requests.

And this is different to their HTTP Adapter which is what this module is using.

I'm wondering if there need to be 2 distinct jest-polly modules? One that handles each of these adapters. Or could they co-exist? If they co-existed then one would also have to ensure that Node >= 18 etc.

@moltar moltar reopened this Feb 17, 2023
@moltar moltar linked a pull request Feb 17, 2023 that will close this issue
@moltar
Copy link
Contributor

moltar commented Feb 17, 2023

Understood! In that case it doesn't seem to be working. The issue appears to be in the adapter. But I didn't really spend too much time on this. See the PR linked above.

I think if the adapter can be made to work, then on this end, we should just either make the adapter configurable. Or not set it at all, since it can be set via a global Polly singleton anyways.

@moltar
Copy link
Contributor

moltar commented Feb 17, 2023

For now, I do not have more bandwidth to dig further at the moment. If you want to propose a fix, I will make sure to review & merge it in a timely manner.

@guyellis
Copy link
Author

I just read your note in the PR and you are running into the same issue I was seeing. It looks like you've tracked down why this is happening and relates to nock and its (lack of) Node 18 support.

Is there a way that we can mark this issue as blocked by nock/nock#2397 ? That way perhaps there'll be a notification when that's resolved and we can do an update request to PollyJS...

In the meantime I think we just leave this open for others like me to see.

@moltar
Copy link
Contributor

moltar commented Feb 17, 2023

Don't think there's a way to set issue deps on GH. But you can subscribe to the issues in the nock repo to receive notifications. See the Subscribe button on the right-hand side.

screenshot-20230217T170205-jbeXUKfp@2x

@jamespacileo
Copy link

stumbling in the same issue :)

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 a pull request may close this issue.

3 participants