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

feat: 🎸 improve yar values and flash typings #169

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

damusix
Copy link

@damusix damusix commented Mar 19, 2024

  • This is a proposition to enforce yar typings
  • Increases visibility into session data
  • Enforces consistent flash types

@damusix damusix changed the title feat: 🎸 improve yar values and flash typings WIP: feat: 🎸 improve yar values and flash typings Mar 19, 2024
@damusix damusix changed the title WIP: feat: 🎸 improve yar values and flash typings feat: 🎸 improve yar values and flash typings Mar 19, 2024
Copy link
Contributor

@Marsup Marsup left a comment

Choose a reason for hiding this comment

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

My suggestions seem to fix the export error while still passing the tests. I think we should include error tests to make sure types are just what they pretend to be

lib/index.d.ts Outdated Show resolved Hide resolved
lib/index.d.ts Outdated Show resolved Hide resolved
@damusix
Copy link
Author

damusix commented Mar 22, 2024

ready now

Copy link

@kanongil kanongil left a comment

Choose a reason for hiding this comment

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

While it makes sense to more strictly type the entries, this approach seems too simple/broken.

Eg. the new generics are pointless since any override must extend from YarValues/YarFlashes. It's not possible to do request.yar.get<{ a: string }>('a') if other custom properties have been added to YarValues.

I'm also missing tests around how set(object) handles errors. Does it even error, or will it accept any object? There are multiple interesting cases here. Also, it would be nice if expect.type() was used to validate the returned types.

@damusix
Copy link
Author

damusix commented Mar 22, 2024

While it makes sense to more strictly type the entries, this approach seems too simple/broken.

Eg. the new generics are pointless since any override must extend from YarValues/YarFlashes. It's not possible to do request.yar.get<{ a: string }>('a') if other custom properties have been added to YarValues.

I'm also missing tests around how set(object) handles errors. Does it even error, or will it accept any object? There are multiple interesting cases here. Also, it would be nice if expect.type() was used to validate the returned types.

Thank you for the feedback it was very insightful. You actually unblocked my though process given what you alluded to about get<{ a: string }>('a'). Looking deeper into it, I even overlooked some issues that don't reflect the reality of how the library works. Currently working on an improvement with better types and tests as you suggested. Check back next week!

@damusix damusix requested review from kanongil and Marsup March 29, 2024 15:39
@@ -109,20 +117,26 @@ declare namespace yar {
/**
* - assigns a value (string, object, etc) to a given key which will persist across requests. Returns the value.
*/
set<T>(key: string, value: T): T;
set<T extends YarValKeys>(key: T, value: YarValues[T]): YarValues[T];
set<T = unknown>(key: string, value: T): T;
Copy link
Contributor

Choose a reason for hiding this comment

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

The PR looks all right, why did you need all those declarations with unknown though?

Copy link
Author

Choose a reason for hiding this comment

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

@Marsup this allowed the autocomplete to pick up <T extends YarValKeys>(key: T, value: YarValues[T]) as well as (key: string, value: T), allowing for typed yar values, and also unknown values. Without the <T = unknown>, it wouldn't pick up any types and set values to any. This was the "AHAH!" moment from @kanongil 's comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I tend to avoid overloads whenever possible as it makes manipulating types much harder, if there's no other way 🤷🏻‍♂️

@Marsup
Copy link
Contributor

Marsup commented Apr 3, 2024

@kanongil does it address your concerns properly?

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.

None yet

3 participants