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

warn when creating a client without url #512

Merged
merged 2 commits into from
Jan 17, 2020

Conversation

JoviDeCroock
Copy link
Collaborator

@JoviDeCroock JoviDeCroock commented Jan 17, 2020

This stems from a twitter conversation where the undefined url would lead to an error in a minimalistic fetch-shim.

Copy link
Member

@kitten kitten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really happy about adding this or other validation since imho you can always take that to an extreme but 🤷‍♂️

@JoviDeCroock
Copy link
Collaborator Author

JoviDeCroock commented Jan 17, 2020

Not really happy about adding this or other validation since imho you can always take that to an extreme but 🤷‍♂️

What would you opt for? I'm pretty positive about developer-friendly warnings for less-obvious things that can result in crashes (obscure crashes for that matter). We shouldn't take it too far but url is pretty essential (it's even 3 out of the 4 letters in urql 🎉)

@andyrichardson
Copy link
Contributor

I like the idea but I think those who want to avoid running into runtime errors like this should use static typing - we have TS types enforcing this.

Maybe a guide on how to get static typing enforcement on a JS code base (basically just creating a tsconfig) is the way to go?

@kitten
Copy link
Member

kitten commented Jan 17, 2020

@andyrichardson this is just a runtime warning, so I think conceptually it’s fine. I think it’s pretty ok given that we can’t enforce every type (full runtime checking in development would just be tedious to add or generate and wouldn’t add much value imho) but my problem with this is, we may as well throw in development? It’s not like this code is going to continue working without a url 😅

@andyrichardson
Copy link
Contributor

@kitten I'm not too opinionated either way, my thinking is related to the cost of maintaining both static and runtime type safety (and keeping them in sync).

Would be awesome if we could respond to issues with "hey, looks like you're not doing any type checking, here's a resource on how you can make sure these kind of issues don't happen on Urql and other type-safe libraries (without re-writing your app in typescript) - https://www.typescriptlang.org/docs/handbook/type-checking-javascript-files.html"

@parkerziegler
Copy link
Contributor

To be fair, I think the issue this spawned out of is here: FormidableLabs/next-urql#24 where an environment variable wasn't coming in correctly. In this case, static typing wasn't the issue (the user's codebase is in TS), but rather an env variable not getting swapped in properly at runtime. This is definitely a good case for this kind of warning, since the error message otherwise (TypeError: Cannot read property 'replace' of undefined) can be a bit difficult to unpack.

@JoviDeCroock
Copy link
Collaborator Author

@parkerziegler I'm fully agreeing with you. Going to merge this for now

@JoviDeCroock JoviDeCroock merged commit 4c75bf7 into master Jan 17, 2020
@JoviDeCroock JoviDeCroock deleted the warn-client-creation-without-url branch January 17, 2020 13:55
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

Successfully merging this pull request may close these issues.

4 participants