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

fix(Types): export more types #6808

Merged
merged 1 commit into from Oct 12, 2021
Merged

fix(Types): export more types #6808

merged 1 commit into from Oct 12, 2021

Conversation

favna
Copy link
Contributor

@favna favna commented Oct 10, 2021

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

Bit annoying that without this we have to copy over types to our own code so lets just export them. Personally (and some other Sapphire users) ran into ExcludeEnum, Souji asked me to also export the collector ones for Yuu.

Then I figured, might as well do a check with /^type / to find any not yet exported types, after this PR everything is exported

Status and versioning classification:

  • I know how to update typings and have done so, or typings don't need updating
  • This PR only includes non-code changes, like changes to documentation, README, etc.

Bit annoying that without this we have to copy over types to our own code so lets just export them

I did a check with /^type / to find any not yet exported types, after this PR everything is exported
@kyranet kyranet added this to the Version 13.3 milestone Oct 10, 2021
@ImRodry
Copy link
Contributor

ImRodry commented Oct 10, 2021

@favna just curious, in what scenarios do you need to use these types yourself?

@favna
Copy link
Contributor Author

favna commented Oct 10, 2021

@favna just curious, in what scenarios do you need to use these types yourself?

ExcludeEnum

Defining ActivitiesOptions[] outside of the instantiation of the Client

function parsePresenceActivity(): ActivitiesOptions[] {
    const { CLIENT_PRESENCE_NAME } = process.env;
    if (!CLIENT_PRESENCE_NAME) return [];

    return [
        {
            name: CLIENT_PRESENCE_NAME,
            type: envParseString('CLIENT_PRESENCE_TYPE', 'LISTENING') as ExcludeEnum<typeof ActivityTypes, 'CUSTOM'>
        }
    ];
}

The collector ones: https://github.com/almostSouji/yuudachi/blob/08cb404b4dece3a09ced34463ae2ea95b5749f2a/src/util/awaitComponent.ts

@ImRodry
Copy link
Contributor

ImRodry commented Oct 10, 2021

For your example you can easily do as ActivitiesOptions["type"] and I'm not sure what the issue is on yuu but the same probably applies as well

@favna
Copy link
Contributor Author

favna commented Oct 10, 2021

For your example you can easily do as ActivitiesOptions["type"] and I'm not sure what the issue is on yuu but the same probably applies as well

Doesn't matter. That's not an argument to not export the types. Besides, hate to be the one to give you a reality check, but you're not a maintainer so it's up to people like Souji, Crawl, Kyra and co to make the call whether they want to merge this and considering Souji specifically asked me to make the collector changes... well you do the math.

@ImRodry
Copy link
Contributor

ImRodry commented Oct 10, 2021

I know, I'm just voicing my opinion as I don't think that a discord API library should be exporting generic type utilities because it seems quite out of the scope of the package. There are other alternatives that work just as fine like the one I just mentioned but I know it's up to them in the end to say whether to merge this or not, I just think it's a bad idea

@iCrawl iCrawl merged commit b474677 into discordjs:main Oct 12, 2021
@favna favna deleted the types/export-enum-types branch October 12, 2021 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants