gql.tada turbo Maximum call stack size exceeded

arjunyel commented 1 month ago

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

[X] 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
[X] Read the docs.
[X] Follow our Code of Conduct

JoviDeCroock commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Closing due to lack of reproduction.

raphael-yapla commented 1 month ago

@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: https://github.com/raphael-yapla/gql-tada-vue-issue/commit/2883b609716cd550aa5ce7d9dea252bdfcd245a2

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 commented 1 month ago

@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 commented 1 month ago

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

arjunyel commented 1 month ago

@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 commented 1 month ago

@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): https://github.com/0no-co/gql.tada/pull/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 (https://github.com/0no-co/GraphQLSP/pull/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 commented 1 month ago

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

kitten commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

@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: https://github.com/0no-co/gql.tada/pull/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 commented 1 month ago

@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 commented 1 month ago

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

arjunyel commented 1 month ago

Thank you friend, added. I am using Node 20.16.0

npm install
npx gql.tada turbo

raphael-yapla commented 1 month ago

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 commented 1 month ago

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

arjunyel commented 1 month ago

@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 commented 1 month ago

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 😄

0no-co / gql.tada