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

Improve typesafety of Snowflake #5157

Closed
wants to merge 9 commits into from
Closed

Improve typesafety of Snowflake #5157

wants to merge 9 commits into from

Conversation

jonahsnider
Copy link
Contributor

@jonahsnider jonahsnider commented Dec 27, 2020

Please describe the changes this PR makes and why it should be merged:

Since Snowflakes are always numbers in string form, this updates the type to use a template literal type, which will grant TypeScript users safe usage of the type.

Status

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Semantic versioning classification:

  • This PR changes the library's interface (methods or parameters added)
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

Copy link
Contributor

@vaporoxx vaporoxx left a comment

Choose a reason for hiding this comment

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

the typescript dependency needs to be bumped to 4.1

Copy link
Member

@kyranet kyranet left a comment

Choose a reason for hiding this comment

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

That... works... I wasn't aware ${number} was a thing:

image

Only thing though, it allows decimals, but it's still better than nothing. Playground here.

Edit: Playground for ${bigint} for 20cbac9, this solved decimals inside numbers, and seems to forbid the n suffix (which BigInts use).

Once CI's errors are solved, this is good to go.

@jonahsnider
Copy link
Contributor Author

Fixed by doing ${bigint}!

typings/index.d.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@NotSugden NotSugden left a comment

Choose a reason for hiding this comment

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

https://github.com/pizzafox/discord.js/blob/cdb6646c9da782461d09a4823924a037aea9f38c/typings/index.d.ts#L2853
this type and possibly others need to be updated to string | Snowflake

a MessageReactionResolvable can also be name:id which does not match `${bigint}`

@ckohen
Copy link
Member

ckohen commented May 10, 2021

Since discord_api_types is now a dependency, this (and many others) could be imports instead

@iCrawl
Copy link
Member

iCrawl commented May 10, 2021

This PR is kind of dead anyway, the requested changes weren't made and @vaporox has a bigger typings PR anyway before we release v13 (currently it was closed because the changes would conflict too much with bigger PRs)

@iCrawl iCrawl closed this May 10, 2021
@jonahsnider jonahsnider deleted the patch-2 branch May 10, 2021 13:07
@iCrawl iCrawl added this to the Version 13 milestone Jun 27, 2021
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

9 participants