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

gql.tada turbo Maximum call stack size exceeded #347

Closed
3 tasks done
arjunyel opened this issue Jul 24, 2024 · 19 comments
Closed
3 tasks done

gql.tada turbo Maximum call stack size exceeded #347

arjunyel opened this issue Jul 24, 2024 · 19 comments

Comments

@arjunyel
Copy link

arjunyel commented Jul 24, 2024

Describe the bug

I've been using the turbo command for a while now. Today I added some new queries and now I am getting a Maximum call stack size exceeded error and I am not even sure how to begin debugging it. I was going to ask in discord but the link https://urql.dev/discord is broken. Thank you for the help!

 ⚠ Unexpected Error 
Could not build cache
┗ RangeError: Maximum call stack size exceeded
      at getNameOfSymbolAsWritten (/test/node_modules/typescript/lib/typescript.js:58431:36)
      at createExpressionFromSymbolChain (/test/node_modules/typescript/lib/typescript.js:55959:27)
      at symbolToExpression (/test/node_modules/typescript/lib/typescript.js:55952:14)
      at symbolToNode (/test/node_modules/typescript/lib/typescript.js:54160:14)
      at /test/node_modules/typescript/lib/typescript.js:54068:144
      at withContext2 (/test/node_modules/typescript/lib/typescript.js:54191:29)
      at symbolToNode (/test/node_modules/typescript/lib/typescript.js:54068:80)
      at symbolToStringWorker (/test/node_modules/typescript/lib/typescript.js:53958:22)
      at usingSingleLineStringWriter (/test/node_modules/typescript/lib/typescript.js:16324:5)
      at symbolToString (/test/node_modules/typescript/lib/typescript.js:53956:62)

Reproduction

No response

gql.tada version

"gql.tada": "^1.8.2",
"typescript": "5.5.4"

Validations

  • I can confirm that this is a bug report, and not a feature request, RFC, question, or discussion, for which GitHub Discussions should be used
  • Read the docs.
  • Follow our Code of Conduct
@JoviDeCroock
Copy link
Member

The discord link works just fine for me 😅 from what I can tell we are stuck in one of these checker calls mind putting a console log there and printing the document in question? You can do that with callExpression.getFullText()

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Jul 24, 2024

That's odd, we see 12 logs of a document and 12 logs of a documentType 😅 Depending on where you logged that it should be a different amount, was trying to lock down where this happens. That being said, I would love a reproduction instead then as remote debugging like this won't work

The only thing I can think of is you have a circular fragment

@kitten kitten added the needs more info ✋ A question or report that needs more info to be addressable label Jul 29, 2024
@kitten
Copy link
Member

kitten commented Jul 29, 2024

Closing due to lack of reproduction.

@kitten kitten closed this as not planned Won't fix, can't repro, duplicate, stale Jul 29, 2024
@raphael-yapla
Copy link

@JoviDeCroock @kitten I'm facing the same issue after updating from an older version of gql.tada. I'm not using the turbo command but it breaks very similarly on the check command.

Here's a reproduction repo with the problem:

https://github.com/raphael-yapla/gql-tada-vue-issue/tree/nested-fragment-composition

I couldn't pinpoint exactly which package/subpackage version breaks the command but you can have a look at this lockfile commit to get a better idea: raphael-yapla/gql-tada-vue-issue@2883b60

This bug is completely breaking gql.tada for us so it would be really appreciated if you give it another look, let me know if I can do anything else to help with this!

@arjunyel
Copy link
Author

@JoviDeCroock @kitten I'm facing the same issue after updating from an older version of gql.tada. I'm not using the turbo command but it breaks very similarly on the check command.

Here's a reproduction repo with the problem:

https://github.com/raphael-yapla/gql-tada-vue-issue/tree/nested-fragment-composition

I couldn't pinpoint exactly which package/subpackage version breaks the command but you can have a look at this lockfile commit to get a better idea: raphael-yapla/gql-tada-vue-issue@2883b60

This bug is completely breaking gql.tada for us so it would be really appreciated if you give it another look, let me know if I can do anything else to help with this!

@raphael-yapla hi friend, thank you so much for making a reproduction! I think you should make a new issue since this one is closed so they might miss your comment

@kitten
Copy link
Member

kitten commented Jul 29, 2024

@raphael-yapla: Cheers! I'll take a look tomorrow. Shouldn't take too long with the reproduction ❤️

@arjunyel: Nah, no worries. Notifications are ongoing without mentions/re-opening being needed, since all participants are subscribed, unless they unsubscribe.

Just to narrow this down. Were you using Vue or Svelte as well by any chance?

@kitten kitten reopened this Jul 29, 2024
@kitten kitten added bug 🐛 Oh no! A bug or unintented behaviour. and removed needs more info ✋ A question or report that needs more info to be addressable labels Jul 29, 2024
@arjunyel
Copy link
Author

@raphael-yapla: Cheers! I'll take a look tomorrow. Shouldn't take too long with the reproduction ❤️

@arjunyel: Nah, no worries. Notifications are ongoing without mentions/re-opening being needed, since all participants are subscribed, unless they unsubscribe.

Just to narrow this down. Were you using Vue or Svelte as well by any chance?

I'm using tada in both a Remix and Nodejs project in an NX monorepo. Thank you for taking a look friend!

@kitten kitten added needs more info ✋ A question or report that needs more info to be addressable and removed bug 🐛 Oh no! A bug or unintented behaviour. labels Jul 30, 2024
@kitten
Copy link
Member

kitten commented Jul 30, 2024

@raphael-yapla: Your issue is quite straightforward. We didn't have a log message for this, but your version of @vue/language-core contained breaking changes that caused us to not transpile .vue files. This PR fixes this and also attempts to add a basic check for the future that'll let us know if this happens again (which would make this a third(?) time of a breaking change in a patch for that package): #353

@arjunyel: If you're not using Vue, your issue is likely unrelated. I've got a WIP change to handle this without a crash (0no-co/GraphQLSP#347) but fundamentally, I don't know what's causing this issue in your case. My suspicion is that an import/definition of a fragment cannot be resolved, but without a reproduction (assuming since you're using Remix, you're not using Vue), I can't tell you for sure what's causing this.

Hence, I'll tag this issue with "needs more info" again, since only one of the two reports here will be resolved by the linked PR (#353)

@raphael-yapla
Copy link

@kitten Thank you so much, always very impressed by your turnaround on things like this!

@kitten
Copy link
Member

kitten commented Jul 30, 2024

No worries! Reproductions always matter and help a lot ❤️ There's a prerelease for this change, since we've just merged this to main, so you can use that to check whether it resolves your issue.

I might hold off on releasing it for a bit until I've got some clarity on what's causing @arjunyel's issue, and which changes we'd like to make to GraphQLSP to let the CLI bail out gracefully in these cases

@arjunyel
Copy link
Author

@kitten yall are amazing! Ill try to work on a reproduction but it might be a while as Im in the middle of a big work project. Does your WIP PR add logging that might help figure out why its mad? If so I'm happy to try out a test version and see what it logs out

@kitten
Copy link
Member

kitten commented Jul 30, 2024

@arjunyel: Not really. I've basically bumped the stack limit (I've just decided to open a PR to also add a small utility for that to do that by default: #355) and then looked at the issue and worked backwards a little.
The main thing we'd need is to understand what's causing the error, where it's thrown, and what the source file looks like to TS (this is admittedly quite hard without a reproduction)

@arjunyel
Copy link
Author

@kitten would you be open to me adding you to my private repo? Then you'll just delete your clone when you're done debugging :) I run npx gql.tada turbo at the project root to get the error

@kitten
Copy link
Member

kitten commented Jul 30, 2024

If you're fine with that, I'm not opposed to that at all

@arjunyel
Copy link
Author

Thank you friend, added. I am using Node 20.16.0

  1. npm install
  2. npx gql.tada turbo

@raphael-yapla
Copy link

No worries! Reproductions always matter and help a lot ❤️ There's a prerelease for this change, since we've just merged this to main, so you can use that to check whether it resolves your issue.

I might hold off on releasing it for a bit until I've got some clarity on what's causing @arjunyel's issue, and which changes we'd like to make to GraphQLSP to let the CLI bail out gracefully in these cases

Happy to say that the pre-release completely resolves my issue! Thanks again!

@kitten
Copy link
Member

kitten commented Jul 30, 2024

@arjunyel: Took a little to track this down, but this comes down to an invalid "moduleResolution" setting in your repository's root tsconfig.json.

Basically, your root tsconfig.json has an entry for moduleResolution: "NodeNext". This is probably (I didn't fully check) overwritten for every single workspace package in your repo, as it's likely not the default you intended to have or extend from. As this mode requires file extensions (as per Node's strict ESM rules), this will cause semantic errors on import statements in several files.

Changing this to Bundler should likely resolve the issue.
GraphQLSP obviously needs some fixes to handle invalid imports, but that's something we'll have to introduce slowly, since it requires some larger refactors.

I'm thinking, for now, we might force override NodeNext as Bundler is basically a more permissible super-set, in theory at least.

In the TSServer (i.e. the language server) different tsconfig.jsons are read anywhere, so for specific sub-folders, the value ends up being correct. However, you can also see when you're running tsc (and you probably want to address some of the other issues once this is fixed) that this is surfaced in there:

apps/frontend/app/queries/user.tsx(7,8): error TS2835: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../utils/matrix-sessions.js'?
apps/frontend/app/queries/user.tsx(8,27): error TS2835: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../utils/UserInfoTypes.js'?

I can probably add an override, since it seems in theory quite safe, but it's probably worth saying that our assumption was (which we may partially address) that the gql.tada CLI's TypeScript-based commands will only behave predictably when no major compilation errors occur. It is somewhat resilient to some type errors, but not resolution errors.

@kitten kitten removed the needs more info ✋ A question or report that needs more info to be addressable label Jul 30, 2024
@kitten kitten closed this as completed Jul 30, 2024
@arjunyel
Copy link
Author

@kitten You are one of the best people in human history, for real. Changing NodeNext to Bundler also fixed a bunch of eslint typescript issues I was having. Thank you so much! I am sorry this ended up being user error.

@kitten
Copy link
Member

kitten commented Jul 30, 2024

Nice! 🙌 I'm hoping we can also fix the crash in GraphQLSP, but that may take me a little longer and a couple more internal chats, so we can make sure it'll end up in a place where we don't break the LSP features while fixing the CLI bugs 😄

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

No branches or pull requests

4 participants