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

test: add remote file test #60

Merged
merged 1 commit into from
Jun 20, 2022

Conversation

ThornWalli
Copy link
Contributor

@ThornWalli ThornWalli commented Mar 23, 2022

Hello :)

have an addition here for the test, there is still missing the retrieval of an external resource.

The whole thing has now but a background ;) we use in our package nuxt-speedkit the @nuxt/image and can not update here for a long time basics/nuxt-booster#521.

This all came with the ipx update to ohmyfetch with the included node-fetch@3.

Could find two problems:

  1. node-fetch update to version 3 brings errors when calling many requests.

Ohne Titel 13

This topic can be found here:
- node-fetch/node-fetch#1474
- node-fetch/node-fetch#1325

And can be easily reproduced with the many call to http://localhost:3000/width_200/https://avatars.githubusercontent.com/u/23360933?s=500 in dev mode.

  1. Another problem I encountered is in the test.
    There I get this error message when running ohmyfetch.fetch and in worst case a segment fault jest.
Error Message Segment Fault
image image

I created a small repo for ohmyfetch, in the test you can see that the call has a problem.

Therefore, this test will probably not be passed in PR.

Could I run it completely if I replaced ohmyfetch with node-fetchor changed the import to:

import { fetch } from 'ohmyfetch/dist/node.mjs'

In both cases, I had to extend the jest configuration with babel, because node-fetch@3 runs in jest only as ESM (node-fetch/node-fetch#1289) and the node.mjs has to be converted as well.

Now I'm asking myself, what's the next step here?

  1. wait for a node-fetch@3 update, where one of the given PRs is included.
  2. the use of ohmyfetch in the jest must work. (rollup issue?)

The easiest solution would be to switch back from ohmyfetch to node-fetch@2.

Thanks already for the help :)

unjs/ofetch#57

@pi0
Copy link
Member

pi0 commented Jun 20, 2022

Thanks for working on this PR and sorry for inconveniences you had with node-fetch versioning. You weren't alone. The latest nuxt is using unjs/node-fetch-native to address most of the problems you had.

@pi0 pi0 changed the title chore(test): add remote file test test: add remote file test Jun 20, 2022
@pi0 pi0 merged commit c06363c into unjs:main Jun 20, 2022
@ThornWalli
Copy link
Contributor Author

@pi0 I have seen that it is now installed, could you update the dependency node-fetch in node-fetch-native?

Have warnings with MaxListeners, which should then be fixed.

@ThornWalli ThornWalli deleted the feature/test-for-remote-file branch June 20, 2022 15:05
@pi0
Copy link
Member

pi0 commented Jun 20, 2022

Sure! You can always make issue in the repos this way I can find it faster.

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.

None yet

2 participants