unjs / ipx

🖼️ High performance, secure and easy-to-use image optimizer.
MIT License
1.54k stars 61 forks source link

test: add remote file test #60

Closed ThornWalli closed 2 years ago

ThornWalli commented 2 years ago

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 https://github.com/GrabarzUndPartner/nuxt-speedkit/pull/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:

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 (https://github.com/node-fetch/node-fetch/issues/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 :)

https://github.com/unjs/ohmyfetch/issues/57

pi0 commented 2 years ago

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.

ThornWalli commented 2 years ago

@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.

pi0 commented 2 years ago

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