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

[Bot] Customize Typings Based on DesiredProperties #2983

Open
Skillz4Killz opened this issue Apr 9, 2023 · 4 comments · May be fixed by #3453
Open

[Bot] Customize Typings Based on DesiredProperties #2983

Skillz4Killz opened this issue Apr 9, 2023 · 4 comments · May be fixed by #3453
Labels
pkg-bot Affects the bot package w-verified This had been verified
Milestone

Comments

@Skillz4Killz
Copy link
Contributor

Is your feature request related to a problem? Please describe.
It would be better to have typings be generated automatically based on the desired properties the user provides.

Describe the solution you'd like

  • Possibly a CLI like npx prisma generate where we do npx discordeno generate
  • Typescript generic that user provides on createTransformers<>() which can provide custom ones

Describe alternatives you've considered

Additional context

With current solution it is not typesafe as a desiredPropety could be set to false and yet the typings say it exists. We could make all properties optional but that isn't exactly a good UX either.

@Skillz4Killz Skillz4Killz added the pkg-bot Affects the bot package label Apr 9, 2023
@MatthewSH
Copy link
Contributor

MatthewSH commented Sep 9, 2023

Would love to hop in on this conversation since it's been a topic of conversation with us on Discord, mainly around desiredProperties. I know our conversation has more geared towards a DD CLI.

I'm not opposed to a CLI, I think a CLI makes sense. It allows those who use TypeScript that type safety. I can say from experience that this threw me for a loop as well, mainly cause I didn't know desiredProperties existed. I was calling properties that TS expected but failed when running.

Having proper documentation, typing, and/or CLI could've mitigated that surprise and frustration for me.

On the CLI generation...
Initial feeling without looking at any other CLIs or generators that exist I think there's a couple options. I'll give a couple thoughts.

JS File

Squashed the details to make this issue shorter

Pros

  • Could be imported/required into the implementation file and be used directly without the need for assert { type: 'json' } using JSON
  • Since it's actual code you could leverage it to create custom transformers or customizers, that is if we're limiting it to just the createTransformers method
  • Has the full power of JS without being limited to primitives (such as JSON, YAML, or TOML)

Cons

  • Lack of typing, since it's JS only but could still leverage JSDoc using import('my-types').Transformers for example? Quick thought there.
  • Would still need to allow importing of JS files in your TSConfig which may not be desired
  • Different languages in one codebase

Discordeno Changes

We would have to change this line to no longer pass in an empty object. We could, depending on the implementation of the config, just pass in the config.

User Side

import myConfig from './discordeno.config.js'

createBot({
  config: myConfig
})

On the Discordeno side we could change it to:

{
  transformers: createTransformers(myConfig.transformers ?? {}),
}

We would also have to change the default desiredProperties

desiredProperties: {
  attachment: {
    id: false,
    filename: false,
    contentType: false,
    size: false,
    url: false,
    proxyUrl: false,
    height: false,
    width: false,
    ephemeral: false,
    description: false,
  },
  // all the other properties here
  // Then spread the options.desiredProperties
  ...options.desiredProperties
}

Doing it this way would allow defaults and protect for future additions while also overwriting the object's properties as a last step. In this case, based on the example below, the entire attachment object would be all true because we saved spread operation to the last step.

Example Config

Using export default would require ES6+ if I recal.

discordeno.config.js

export default {
  transformers: {
    desiredProperties: {
      attachment: {
        id: true,
        filename: true,
        contentType: true,
        size: true,
        url: true,
        proxyUrl: true,
        height: true,
        width: true,
        ephemeral: true,
        description: true,
      }
    }
  }
}

JSON

Squashed the details to make this issue shorter

Pros

  • Configuration through JSON is normal, although less so with things like Webpack and others? At least normal from personal experience
  • It's known and can be restrictive to primitive values, can be good or bad.
  • You can provide JSON schemas which most popular editors support in some way to real time validate the schema, you can even version it if you want.

Cons

  • Is it restrictive, wouldn't give DD flexibility like you could with JS implementation since it would be restricted to primitive JSON types. So no custom transformers.
  • You'd have to use assert { type: 'json' } wherever you import it
  • You'd have to allow JSON modules in TSConfig
  • Could potentially cause typing issues, just a theory
  • Biggest Con is that the $schema would have to be maintained or generated on release which is another point of potential failure or extra time spend

Discordeno Changes

Same as the JS implementation as I previously documented. That's going to be more or less the same for both. User side would be similar, just adding the assert { type: 'json' } instead of importing the file.

Example Config

{
  "$schema": "https://some.schema/discordeno-19.0.0.json",
  "transformers": {
    "desiredProperties": {
      "attachment": {
        "id": true,
        "filename": "true"
      }
    }
  }
}

I won't list out all the items.

My Opinion

My personal opinion is to go the JS route. CLI could look like:

npx discordeno generate types

We could also create a shortcut with dd so instead of npx discordeno we could use npx dd.

Inside the CLI we could look for a discordeno.config.js file and require it in and generate a typing file based of it.

Copy link

github-actions bot commented Dec 4, 2023

This issue has gone stale for over a month. If this issue is useful, leave a comment below. Otherwise, it will be closed shortly.

@H01001000 H01001000 added this to the v19 milestone Dec 7, 2023
@MatthewSH
Copy link
Contributor

Bump for stale bot

@Fleny113 Fleny113 added the w-verified This had been verified label Feb 22, 2024
@Fleny113 Fleny113 linked a pull request Feb 22, 2024 that will close this issue
28 tasks
@Fleny113
Copy link
Contributor

I want to get some more work on this done:

Currently the totally blocking issues is supporting Deno. This is because without the node_modules folder we can't go "the prisma route", which consists of modifying your node_modules.
However, as a workaround we can generate a plugin1 that will override a bunch of the typing, including the bot.transformers, bot.helpers and bot.events, this is needed because since we can't override the types themself afaik, we need to replace them with the correct variant.

This causes a few issues that needs to be solved:

  1. If a plugin1 was to add any helper/transformer/event method there would be some not so nice type errors. If the desired proprieties plugin was the last one to get invoked then the other plugin helpers would get removed unless put in a separate object from the 3 objects that we need to override types for, and even then, there would be an issue because you can't get the type that has the desired proprieties.
    • This complicates stuff even further because if a generic library/util accept a Message the object with the desired proprieties would not be accepted by typescript because of the incompatibile proprieties.
  2. Currently in feat(cli): CLI generated types #3453 there is a giant JavaScript object that is really difficult to navigate and add or edit stuff in, however this is solvable by using the Typescript compiler API, but this can be done either at runtime or build time. When i say runtime what i mean the single CLI invocation, but this has the issue (with Deno2 especially) on how to get TS to know the types of our types. With build time I mean on our build script, which would require adding a script to generate a .generated.ts file with all the information for then allowing the CLI to create a .generated.ts at runtime for the user.

Warning

The Typescript compiler API is considered experimental by the Typescript team and there is no guarantee of compatibility between minor versions, however they do document the breaking changes afaik

  1. In this specific issue, there was a small discussion on how the config format should work and how it should be resolved

Footnotes

  1. I want to be clear with what i mean when saying "plugin", what i mean is when we have a function that accepts a Bot object, it modifies it in any way and it gives it back to the user. 2

  2. Why is it always Deno? XD, I'm aware that it is in no way intentional by the Deno team prevent code gen, (i assume) however at least at this time is what is causing the most issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg-bot Affects the bot package w-verified This had been verified
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants