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

Typescript: More specific types for children #35

Closed
osdiab opened this issue Apr 24, 2020 · 8 comments
Closed

Typescript: More specific types for children #35

osdiab opened this issue Apr 24, 2020 · 8 comments
Labels
enhancement New feature or request

Comments

@osdiab
Copy link

osdiab commented Apr 24, 2020

This is not necessarily the right place to put this since typings are managed at DefinitelyTyped, but figured it would get more relevant eyeballs here so posting:

One of the great things about MJML is that it validates your email HTML. But it would be even better if some of those validations could occur at compile time instead.

There's a lot of low hanging fruit like "children of mj-sections need to be mj-columns" or "an h1 tag can only be inside of an mj-text instance", which can be checked before even running your code. Would be amazing if those were typed explicitly - although I am not sure how to type "an instance of a specific component" off the top of my head yeah i don't think it's possible :P

Exploring other ways it can potentially be done without messing with the API of this library

@osdiab
Copy link
Author

osdiab commented Apr 24, 2020

Made an issue to see if it can be done at the lint level but I'm not optimistic :P doing this might require doing a non-JSX structure to make it easier to type, so maybe a fork of this library

@osdiab
Copy link
Author

osdiab commented May 18, 2020

Can def happen at runtime with proptypes, and Flow can type it - but TypeScript is unable at the moment (possibility for late june Typescript 4.0 (microsoft/TypeScript#38510 (comment)), but the PR for it has perf issues so potentially not) - discussions at typescript here:

@daliusd
Copy link
Contributor

daliusd commented Sep 23, 2020

Typescript allows checking children type https://www.typescriptlang.org/docs/handbook/jsx.html#children-type-checking . I guess this might be used.

@osdiab
Copy link
Author

osdiab commented Sep 24, 2020

@daliusd that doesn't help unless the children are not actually react elements but just objects or primitives that mjml-react can use to build the react elements themselves, but that would be a dramatic and very breaking change to this library. for instance if you set the children prop to be an array of strings like {children: string[]} that works, but you can't have the children be specific instances of react components since the type of <AnyComponent /> will just be a JSX.Element type, which aren't distinguishable.

@osdiab
Copy link
Author

osdiab commented Sep 24, 2020

for example to use that, instead of writing an MJML component like

<MjmlSection>
  <MjmlColumn />
</MjmlSection>

You would need to not use JSX for the children and do something like this:

<MjmlSection>
  {[
    {type: "MjmlColumn", contents: []}
  ]}
</MjmlSection>

and MjmlSection would need to know how to transform that children array into actual JSX elements. But that's very clumsy by React standards and I think would be best supported by just not using this library altogether lol

@o-alexandrov
Copy link

If project owners are open to migration to TypeScript, I could help migrating all JavaScript into TypeScript in a PR.

  • just let me know, so I don't start something that you are not okay with :)

@mastertheblaster
Copy link
Collaborator

Hey @o-alexandrov we are thinking about converting this project to a typescript. We are doing a little bit of research currently. Once finished we will let you know. Stay tuned.

@mastertheblaster mastertheblaster added enhancement New feature or request help wanted Extra attention is needed and removed help wanted Extra attention is needed labels Oct 20, 2020
@daliusd
Copy link
Contributor

daliusd commented Sep 5, 2022

Hey, this project is no longer maintained so if someone wants to do something about it then feel free to fork it.

@daliusd daliusd closed this as completed Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants