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

chore: get rid of UnsetMarker (ts challenge) #3609

Closed
1 task done
KATT opened this issue Jan 12, 2023 · 4 comments
Closed
1 task done

chore: get rid of UnsetMarker (ts challenge) #3609

KATT opened this issue Jan 12, 2023 · 4 comments
Labels
🧙🏻 typescript wizardry if you want a typescript-challenge, look at these

Comments

@KATT
Copy link
Member

KATT commented Jan 12, 2023

Describe the feature you'd like to request

UnsetMarker is a Symbol and our way of defining that something is unset.

We mark procedures' inputs etc as in its initial state.

It's a lot easier to do extends UnsetMarker than extends undefined etc given that undefined is actually a valid value.

Describe the solution you'd like to see

I am not sure we need it. There might be a neater way?

The challenge is to search through the codebase for UnsetMarker and try to get rid of it.

Describe alternate solutions

Not doing it. It might not be worth caring about

Additional information

It was added as a shortcut to some type of issues that I can't remember.

👨‍👧‍👦 Contributing

  • 🙋‍♂️ Yes, I'd be down to file a PR implementing this feature!
@KATT KATT added the 🧙🏻 typescript wizardry if you want a typescript-challenge, look at these label Jan 12, 2023
@omermizr
Copy link
Contributor

omermizr commented Jan 14, 2023

Hey @KATT, first off I wanna say I love this project and keep up the good work!

I dug through the codebase for references of UnsetMarker and here is my 2 cents:

  1. It is possible and fairly easy to remove UnsetMarker in favor of undefined, but I'm not sure it'd be the right approach. It would make it impossible to differentiate between an unset state, and someone explicitly wanting undefined as a value.
    While I can't think of a real-world use-case right now, I still think it's a good idea to be explicit about the default state. Here a (somewhat contrived) example:
    t.procedure.output(z.undefined()).query(() => 'foo'); // should error because 'foo' is not undefined
    
  2. We can eliminate the runtime symbol and just use a type symbol.
  3. I don't see a real issue with using UnsetMarker internally, however it's not ideal if it bleeds into user applications, which leads me to;
  4. There is currently a bug where a procedure with no inputs will receive an UnsetMarker as the input to the query/mutation etc.

I'd be happy to submit a PR for 2 and 4 if you decide that's the direction you wanna go in :)

@KATT
Copy link
Member Author

KATT commented Jan 14, 2023

  1. Won't work due to the fact that undefined is a possible input and sometimes we need to distinguish between explicit undefined and set undefined
  2. I think the symbol is tree shaken already - it's at least designed in a way where it shouldn't be possible to import it externally
  3. Fair, it might be a non issues!
  4. Oh shit, I didn't know, could you make a failing test that shows this?

@omermizr
Copy link
Contributor

omermizr commented Jan 14, 2023

  1. Exactly
  2. 👍
  3. 👍
  4. Here you go: fix: do not leak UnsetMarker to query input #3614. It's just the test for now, but I can add a fix too

@KATT
Copy link
Member Author

KATT commented Jan 14, 2023

I'll just close this as a non-issue for now. Nice it led to you highlighting that issue, @omermizr, tyvm!

@KATT KATT closed this as not planned Won't fix, can't repro, duplicate, stale Jan 14, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🧙🏻 typescript wizardry if you want a typescript-challenge, look at these
Projects
None yet
Development

No branches or pull requests

2 participants