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

typings: mark enums as const enums #6394

Merged
merged 2 commits into from
Aug 24, 2021
Merged

typings: mark enums as const enums #6394

merged 2 commits into from
Aug 24, 2021

Conversation

danny-may
Copy link
Contributor

Please describe the changes this PR makes and why it should be merged:
The enum instances arent actually exported, so shouldnt be declared in the typings as being exported.
Declaring them as const enums instead means they will be replaced by their value at compile time, removing any runtime errors when trying to access the enum value.

This means importing discord.js/typings/enums and using the enum will no longer result in a runtime error, without impacting existing code or definitions.

Status and versioning classification:

  • 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

@DTrombett
Copy link
Contributor

DTrombett commented Aug 11, 2021

This will probably create the same problem as #6078

@danny-may
Copy link
Contributor Author

danny-may commented Aug 11, 2021

I dont see how as you cant currently use indexers on these types because they arent exported, you get a runtime error if you try.

I do think it would be best of the enums members were exported and these enums werent converted to const enums, but failing that this is the next best thing as it removes the discrepancy between compile time and runtime

@suneettipirneni
Copy link
Member

I dont see how as you cant currently use indexers on these types because they arent exported, you get a runtime error if you try.

#6396 wink

@danny-may
Copy link
Contributor Author

@suneettipirneni +1 for that, but youll still have the same error at runtime because the index.js file doesnt export the enums. If youre allowed to add them to the index.js file then ill close this pr in favour of yours

@suneettipirneni
Copy link
Member

@suneettipirneni +1 for that, but youll still have the same error at runtime because the index.js file doesnt export the enums. If youre allowed to add them to the index.js file then ill close this pr in favour of yours

ohhh, lemme look into that

@iCrawl
Copy link
Member

iCrawl commented Aug 11, 2021

Neither of those 2 PRs will work if we don't export the enums in our code. We tried this before, there have been 3-4 PRs on this topic already.

@danny-may
Copy link
Contributor Author

Ive tested this pr in my bot and it works fine without the enum being exported in the js because const enums are a compile time only structure. They get replaced by actual values at runtime

@iCrawl
Copy link
Member

iCrawl commented Aug 11, 2021

This still holds true:

#6065 (comment)
#6078 (review)

I don't see how this PR handles this.

@danny-may
Copy link
Contributor Author

danny-may commented Aug 11, 2021

The issue with #6065 (comment) is that the typescript declaration was declaring there to be an instance available at runtime when there wasnt, hence the undefined you get at runtime. Const enums dont declare anything as existing at runtime. As a toy example:

enum ThisEnumExistsAtRuntime {
    OPTION_1 = 1,
    OPTION_2 = 2,
    OPTION_3 = 3
}

const enum ThisEnumDoesntExistAtRuntime {
    OPTION_A = 'A',
    OPTION_B = 'B',
    OPTION_C = 'C'
}

const x = ThisEnumExistsAtRuntime.OPTION_1 + ThisEnumExistsAtRuntime.OPTION_2;
const y = ThisEnumDoesntExistAtRuntime.OPTION_A + ThisEnumDoesntExistAtRuntime.OPTION_B;

compiles to

"use strict";
var ThisEnumExistsAtRuntime;
(function (ThisEnumExistsAtRuntime) {
    ThisEnumExistsAtRuntime[ThisEnumExistsAtRuntime["OPTION_1"] = 1] = "OPTION_1";
    ThisEnumExistsAtRuntime[ThisEnumExistsAtRuntime["OPTION_2"] = 2] = "OPTION_2";
    ThisEnumExistsAtRuntime[ThisEnumExistsAtRuntime["OPTION_3"] = 3] = "OPTION_3";
})(ThisEnumExistsAtRuntime || (ThisEnumExistsAtRuntime = {}));
const x = ThisEnumExistsAtRuntime.OPTION_1 + ThisEnumExistsAtRuntime.OPTION_2;
const y = "A" /* OPTION_A */ + "B" /* OPTION_B */;

If you declare export enum ... then typescript expects there to be something there at runtime. The current declarations do this when nothing is exported, so something should be done to fix this. Const enums are designed for exactly this situation where you have a logical enum that isnt exported.

I can change this PR to expose them as interfaces if that would be preferable. It would compile to the exact same code, but would be more annoying to use as in this comment: #6065 (comment)

In essence this PR is to remove export enum ... as its causing a compiletime/runtime discrepancy. For the sake of keeping changes to a minimum, I went with the closest alternative which is export const enum .... export interface ... and export type ... will both solve the core issue too, but will be less elegant about it.

Copy link
Member

@SpaceEEC SpaceEEC left a comment

Choose a reason for hiding this comment

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

Making those const enum breaks:

  • non-literal string access, e.g. ChannelTypes[channel.type] (errors, but in runtime returns a number)
  • Reverse mapping, e.g. ChannelTypes[ChannelTypes.GUILD_TEXT] (errors, but in runtime returns a string)

We actually export those enums under Constants (end of declaration):

export const Constants: {

If there is a way to have them, but not be importable from outside of the project, that would be a solution here, but not making them const enums.

@danny-may
Copy link
Contributor Author

danny-may commented Aug 11, 2021

Good point! Ive updated the PR to support that. These are the test cases I used:

import { Channel, Constants } from 'discord.js';
import { ChannelTypes } from 'discord.js/typings/enums';

const channel: Channel = <Channel><unknown>undefined; // Hack just to get the type info for testing

const x = Constants.ChannelTypes[channel.type];              // type: ChannelTypes
const y = Constants.ChannelTypes[Constants.ChannelTypes.DM]; // type: string
const z = Constants.ChannelTypes.DM;                         // type: ChannelTypes.DM
const a = ChannelTypes.DM;                                   // type: ChannelTypes.DM

When using export enum this compiled to

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
const discord_js_1 = require("discord.js");
const enums_1 = require("discord.js/typings/enums");
const channel = undefined;
const x = discord_js_1.Constants.ChannelTypes[channel.type]; // type: ChannelTypes
const y = discord_js_1.Constants.ChannelTypes[discord_js_1.Constants.ChannelTypes.DM]; // type: string
const z = discord_js_1.Constants.ChannelTypes.DM; // type: ChannelTypes.DM
const a = enums_1.ChannelTypes.DM; // type: ChannelTypes.DM
//# sourceMappingURL=index.js.map

and with my changes it compiled to

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
const discord_js_1 = require("discord.js");
const channel = undefined;
const x = discord_js_1.Constants.ChannelTypes[channel.type]; // type: ChannelTypes
const y = discord_js_1.Constants.ChannelTypes[discord_js_1.Constants.ChannelTypes.DM]; // type: string
const z = discord_js_1.Constants.ChannelTypes.DM; // type: ChannelTypes.DM
const a = 1 /* DM */; // type: ChannelTypes.DM
//# sourceMappingURL=index.js.map

I think this satisfies all right? Types and their usage remain unchanged from before this PR but typescript no longer attempts to require("discord.js/typings/enums");

Copy link
Member

@vladfrangu vladfrangu left a comment

Choose a reason for hiding this comment

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

I still believe we could just make our enums proper enums, and export them, but I suppose this is the best of both worlds?

@iCrawl
Copy link
Member

iCrawl commented Aug 12, 2021

This needs a rebase.

@danny-may
Copy link
Contributor Author

This needs a rebase.

Done!

@iCrawl iCrawl added this to the Version 13.2 milestone Aug 13, 2021
@suneettipirneni
Copy link
Member

If this is merged how will this affect Constants? Will it not be needed anymore?

@danny-may
Copy link
Contributor Author

You will still need to use constants if you want to do something like Constants.ChannelTypes[channel.type], otherwise you could use either the enum directly or Constants and have the same result

Copy link
Member

@SpaceEEC SpaceEEC left a comment

Choose a reason for hiding this comment

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

I don't really see the point, but if everything works. 🤷

@iCrawl iCrawl changed the title types(Enums): mark enums as const enums typings: mark enums as const enums Aug 24, 2021
@iCrawl iCrawl merged commit 5c27639 into discordjs:main Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants