trustfall icon indicating copy to clipboard operation
trustfall copied to clipboard

Schema parse errors aren't correctly passed to javascript

Open u9g opened this issue 1 year ago • 7 comments

When a schema implementing another schema fails to re-say that it has a field that a parent does, this is a MissingRequiredField Error (not sure why?) anyways, when this happens in the browser_based_querying repo, an uncaught exception: Object is thrown with no clear indication as to what went wrong. Anything more informative is better, I had to just copy my schema to the rust example and then the rust example happily spit out thread 'main' panicked at 'valid schema: MissingRequiredField("GithubPullRequest", "Webpage", "url", "String!")', trustfall/examples/hackernews/adapter.rs:19:59 which helped me find and fix the problem.

u9g avatar May 12 '23 04:05 u9g

The WASM / Typescript bindings are more of a functional proof of concept than a polished package — that's why I haven't published them to npm, for example. Currently, they work well enough to produce a working playground site, and that's about it. The ergonomics leave a lot to be desired as your issue correctly points out.

Basically: too much surface area in the project, not enough time 😅

I'd welcome PRs to make it all better!

P.S.: Types must re-state the properties and edges they contain because of variance: the same property or edge might have a different (narrower) type on a child vertex type compared to the parent vertex type. For example: a parent vertex type may have value: String but a child type may declare it as value: String! instead. Variance can also exist without changing the type — it can be semantic, where the same edge means something more on a child than on the parent. This distinction would be explained with a docstring on the field. Because of all this, it's better from a design perspective to force this repetition, since there may be variations.

obi1kenobi avatar May 12 '23 05:05 obi1kenobi

@obi1kenobi Could I take this one on if its still an issue? Never have worked with WASM before so it may be an interesting learning experience :)

devanbenz avatar Feb 13 '24 13:02 devanbenz

Please do! Especially if you have JS experience, it would be great to have your help here. The playground is a bit held together by string and duct tape right now 😅

obi1kenobi avatar Feb 13 '24 14:02 obi1kenobi

Perfect! I write typescript/javascript professionally so it's probably my most comfortable language :D

devanbenz avatar Feb 13 '24 14:02 devanbenz

They are by far the ones I'm worst at, so if you'd like to co-maintain the Trustfall JS/TS integration, it's yours! 😅

obi1kenobi avatar Feb 13 '24 14:02 obi1kenobi

Excellent - might need some context on where to start looking for this fix as well. I'm assuming its in the trustfall_wasm package?

devanbenz avatar Feb 13 '24 20:02 devanbenz

I'd probably start by overhauling how trustfall_wasm does testing. It should have a proper TS test setup instead of the atrociously janky "inject hand-written inline javascript via Rust" approach the trustfall_wasm/tests directory currently has. Feel free to use your judgment about how to structure the tests or the APIs under test — I know next to nothing about TS/JS so your call is better than mine.

After replicating the basic functionality test there, I'd continue by adding more test coverage in trustfall_wasm over various edge case scenarios:

  • invalid schema
  • invalid query
  • invalid variables for the query
  • adapter written in TS throws an error in the middle of the iterator of one of its resolver methods etc.

Then, after we're sure that all the various cases work fine in trustfall_wasm, either this issue will have been fixed, or the issue is somewhere in the playground's code.

obi1kenobi avatar Feb 13 '24 21:02 obi1kenobi