NextJs 13.4.1

[CristianSpatari]

Bug Report

Describe the bug

I can't use yahooFinance inside app/* inside use client & use server

Minimal Reproduction

await yahooFinance.search('AAPL') await yahooFinance.quoteSummary('AAPL') ...

Environment

Browser or Node: Node Node version (if applicable): 18 Npm version: 9.5.1 Browser verion (if applicable): Chrome 114.0.5735.133 Library version (e.g. 1.10.1): 2.4.1

Additional Context

[gadicc]

Hey @CristianSpatari

Thanks for the report. Unfortunately I don't have anytime to look into this in depth, but I'll give some comments that may help.

1) fetchDevel.js is a dynamic / conditional import, which is only ever called during tests... it's really, really odd that next is trying to import it, but I guess it has special for app/* files which I guess have some advantage but here is breaking the spec.

2) I haven't ever used NextJS's app structure yet. Is it possible you could put the code that interacts with yahoo-finance back in src and call it from there? Or does that limit the rest of your code to src too?

3) Its unlikely you want to use node-yahoo-finance on the client. It is possible with some work but in any case you would need to proxy the request through some server to avoid CORS restrictions.

Hope this helps and please do report back. Sorry I don't have more time to look at this now.

[jayehernandez]

Hey @gadicc @CristianSpatari, facing the same issue. I tried commenting this out and it seemed to fix it (temporarily on my local)

NextJS's app structure is the main directory, there's no more src directory. I'm trying this out on the server side. And with that commented out it works fine. I did go through the trace and found that there is really no tests directory in the installed package. Is this the intended behavior? If fetchDevel.js is only used in tests, could it be possible to maybe somehow import it elsewhere? It's odd though that it only errors out in NextJS.

[gadicc]

Hey @jayehernandez, thanks for chiming in.

Well, that line is already a dynamic import, that is only imported when needed (i.e. when fetchDevel() is called). It should not be imported otherwise. My guess is that NextJS with app routes, tries to process everything anyway to prepare the bundle for such an import, but it really shouldn't be executing any of that code.

I think this is most likely a NextJS bug, but maybe you can do me a favour since you already have that file edited. Just insert a new line at the top of fetchDevel():

  throw new Error("Oops!  fetchDevel() was called.")

And see if that error is ever thrown. If it is, I think it's a bug on our side, and if not, I think it's a bug on NextJS's side.

That's not to say we can't work around the issue on our side. And fetchDevel() needs to work not only for our tests but for anyone else who wants to build tests working with the package. I'll take a look at it when I get a chance but I'm pretty full up at the moment :/

But so far as I know, you can still use the src / pages directory alongside the app directory, both can co-exist. app is the new recommended way but src / pages is still available for incremental adoption. Assuming you're using yahoo-finance2 in an API route, I think the easiest way to get moving now is to simply run it from there so it will be analyzed / compiled in the old way.

But please keep me posted... will look at this when I have a chance.

[jayehernandez]

Hi @gadicc thanks for this. Compeletely understand, I have resorted to using pages for now. No rush into looking at this, with the workaround it works well.

[gadicc]

Amazing, thanks so much, @jayehernandez; greatly appreciate the feedback :raised_hands:

[theodufort]

Having the same issue with NextJS 13

[z3nful]

It seems that NextJS is looking 1 folder higher than where the tests folder actually is.

I think its due to how they are using the /dist folder instead of the base package source code, since ../../ from fetchDevel.js in that case would put it looking for the tests folder in /dist/esm, when the tests folder is actually located a folder lower from there in /dist.

I think I can fix this in a couple of days as well, once I have a bit of free time at my home PC.

[z3nful]

I was able to setup a prototype fix for this at the following fork branch, but wont be able to attempt to replicate the issue for a while. If someone that is actively using the library under these conditions could test it out, that would be cool :)

https://github.com/z3nful/node-yahoo-finance2-bug-fixes/tree/nextJS-13.4.1

[gadicc]

Oh wow, nice catch! Thanks as always for your valuable contributions here :pray:

Ok sounds great! I hope someone who has been experiencing this issue will be able to test and confirm.

(Also, irrelevant for now, but just to keep you posted with the long term plans, the fetchDevel stuff has worked so well that I made it into it's own library... https://www.npmjs.com/package/jest-fetch-mock-cache. Instead of calling directly like we do in yf2, it's all via jest mocks, so that would also solve any issues certain bundlers might be having.)

[z3nful]

Nice, I'll take a look at it and see if I can incorporate it in any of my current projects so I can provide feedback :) Might even use it for the project I'm using this package for lol

[gadicc]

Thanks! Sure, yeah, feedback welcome. It's my go to package now every time I'm developing new code around an API, and amazing for test driven development. The new approach via jest mocks is much easier to work with, although still one or two minor features missing vs fetchDevel.

[AngelNBazan]

Hi @gadicc, Just tested out this PR and I can confirm it fixed the issue. Thanks, again @z3nful 🫡. Just one thing to note: since fetchDevel.js uses fs it cannot be used in client components.

[pqhuy98]

Thanks for resolving this bug. When will it be available in the next release?

[gadicc]

Hey @AngelNBazan, thanks for checking out the code and confirming, that's a great help! And of course, again, to @z3nful for all his awesome work on this and other issues here.

@AngelNBazan, confirming that fetchDevel.js cannot and should not be used in the browser... it's definitely node / filesystem dependent. It should also only be used in development. In any case, it's a dynamic import that we only try import on first use.

@pqhuy98 I'd like this to be in the next release however I noticed some issues with the proposed fix that will take a bit longer to fix; however, now that @z3nful has identified the issue and a fix, and @AngelNBazan has confirmed it works, we're in a much better position. Now we just need to implement it in a way that both fixes this issue but also doesn't break other things.

@z3nful, sorry, I jumped the gun a bit by a creating a PR from your sample code. It seems to be breaking tests. I should have a chance to look into this week, or let me know if you want to and are free to continue working on it :pray: :pray:

[gadicc]

Sorry, looks like semantic-release doesn't handle reverted commits... this isn't fixed yet :sweat_smile:

(I deleted the previous "fixed" / "release" message)

[orelhochenboym]

Also waiting on this to be resolved :)

[z3nful]

I'll be able to look into this in the next few days, I'm sure once I have time to spend on it I can get it sorted out 🙂

[orelhochenboym]

Is there a status on this?

[z3nful]

I unfortunately haven't had time available to look into the test cases that are failing for this and create a pull request for the changes, but seeing as how this error pops up specifically from fetchDevel.js on library load, it should still be usable with the latest version of the library if you were to replace the following in /{Your App Project Directory}/node_modules/yahoo-finance2/src/lib/fetchDevel.js.:

const BASE_URL = new URL("../../tests/http/", import.meta.url);

with:

const BASE_URL = () => {
  const pathsToCheck = ["../../tests/http/", "../../../tests/http/"];
  for (const path of pathsToCheck) {
    try {
      if (fs.existsSync(path)) {
        return new URL(path, import.meta.url);
      }
    } catch (err) {
      throw err;
    }
  }
  throw new Error("tests folder not found.");
};

One caveat to this is that if you reinstall or update the library, you'll have to make this change again unless the update included where it passed all test cases.

[nocodehummel]

@z3nful thanks for the development workaround, unfortunately this still block Heroku deployment.

[nocodehummel]

This error occurs in my build in a React-server-component (Nextjs v14.0.2).

[gadicc]

Big thanks to @nordicgit70 for taking a stab at this while @z3nful and I have been super bogged down with other stuff!

This looks like a great way to finally fix this issue but hasn't been extensively tested yet. It will be in the next release (and there'll be an automated message here when it finishes building / releasing from CI) so we'd be grateful for some feedback from those in the thread if it solves the problem in various environments.

[gadicc]

:tada: This issue ~has been~ may be resolved in version 2.9.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket:

[nocodehummel]

Release v2.9.0 resolved the Nextjs with React Server Component build issue.

[gadicc]

:fire: :rocket: :pray:

[orelhochenboym]

Like @nordicgit70 said it works great in React Server Components. Unfortunately while using the modules in client components it seems to break. I need to further investigate this matter on my end, but I got these so far while using them inside React Client Components.

Search:

Quote:

[AngelNBazan]

@orelhochenboym Nextjs currently doesn't support async/await in client components, so you would not be able to use this in combination with the useSearchParams client hook. I'm not too sure on a workaround, I would recommend looking at the Nextjs docs this might be helpful docs.

[nocodehummel]

@orelhochenboym you can define an async function inside useEffect.

[orelhochenboym]

@orelhochenboym Nextjs currently doesn't support async/await in client components, so you would not be able to use this in combination with the useSearchParams client hook. I'm not too sure on a workaround, I would recommend looking at the Nextjs docs this might be helpful docs.

I'm not using useSearchParams. It seems to be in the search module, although the quote module throws an error as well.

@orelhochenboym you can define an async function inside useEffect.

I'm already doing that as shown here:

The only difference is the getSearch(symbol:string) function: sends a request to a server I wrote (which then uses the search module) instead of using it instead.

When sending a request to my own server it works, but when using the api directly it throws these errors.

[AngelNBazan]

@orelhochenboym I'm attempting to recreate the errors, so far this is the only way I have been able to replicate it.

I tested the commented code getQuoteData, callData, and it has worked. So my guess is it has to do with how your async fetchData function and how it's being used in client components.

[orelhochenboym]

What seems to work is using a server action.

This works and I receive a proper search response, but I get this error: