[mjml-react] Add Explicit types for children #56254

eps1lon · 2021-10-06T18:09:32Z

We plan to remove implicit children from @types/react. The following changes are required to pass #56210.

typescript-bot · 2021-10-14T13:49:58Z

@eps1lon Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

mjml-react — on npm, on unpkg
- owner-approval: @IanEdington

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

You can test the changes of this PR in the Playground.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
✅ Most recent commit is approved by type definition owners, DT maintainers or others

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 56254,
  "author": "eps1lon",
  "headCommitOid": "a4ed8d569cd9c390609774f65e7a932ebbafb85e",
  "lastPushDate": "2021-11-05T16:16:40.000Z",
  "lastActivityDate": "2021-11-08T09:24:55.000Z",
  "mergeOfferDate": "2021-11-08T02:39:07.000Z",
  "mergeRequestDate": "2021-11-08T09:24:55.000Z",
  "mergeRequestUser": "eps1lon",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "popularityLevel": "Well-liked by everyone",
  "pkgInfo": [
    {
      "name": "mjml-react",
      "kind": "edit",
      "files": [
        {
          "path": "types/mjml-react/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/mjml-react/mjml-react-tests.tsx",
          "kind": "test"
        }
      ],
      "owners": [
        "henrinormak",
        "IanEdington"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "IanEdington",
      "date": "2021-11-08T02:38:32.000Z",
      "isMaintainer": false
    }
  ],
  "mainBotCommentID": 943377354,
  "ciResult": "pass"
}

typescript-bot · 2021-10-14T13:50:00Z

🔔 @henrinormak @IanEdington — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

IanEdington · 2021-10-14T15:49:31Z

IMO there's isn't enough detail about this change to evaluate what the goal is.

This also changes tests in undesirable ways.

Can you provide more info on this change?

eps1lon · 2021-10-14T17:34:22Z

IMO there's isn't enough detail about this change to evaluate what the goal is.

What details are you missing? I've provided a link to the overall goal and describe each individual component. I don't know what else I could provide.

This also changes tests in undesirable ways.

Could you point to specifics? I'm not familiar with what you desire in your tests.

IanEdington · 2021-10-14T19:48:16Z

types/mjml-react/mjml-react-tests.tsx

-    const maxProps: React.ReactNode = <MjmlBreakpoint width="">child</MjmlBreakpoint>;
+    const maxProps: React.ReactNode = <MjmlBreakpoint width="" />;
 }
 // TestMjmlBodyTag
 {
-    const minProps: React.ReactNode = <MjmlBody />;
+    const minProps: React.ReactNode = <MjmlBody />; // $ExpectError


The tests shouldn't change should they?

In general, they should. Specificaly, they change in any place where no children where passed, even though that would throw at runtime,

@IanEdington Please see https://github.com/wix-incubator/mjml-react/blob/7894e20e52db6340e8bad2cbb731f6a3f2430db4/src/mjml-body.js#L8.

<MjmlBody /> would trigger prop-types warnings. Usually the TS types follow the prop-types. Is there any reason why we wouldn't do this here?

Yeah that's confusing. the mjml-react library isn't the most consistent unfortunately.

There is a case where the MjmlBody has to have no children:

export default function () { return ( <Mjml> <MjmlHead> <MjmlAttributes> <MjmlBody background-color="#FFFFFF" /> </MjmlAttributes> <MjmlHead> <Mjml> ); }

I'll put in a PR on that repo.

Since we agree that the types match, can we merge this PR? If the maintainers decide to accept your PR we can always adjust the types.

no because it will break existing valid typescript implementations. The prop-types are often not used since this is usually used as a server side react library to generate html. Emails that are written like this currently are valid in MJML which is the source of truth both mjml-react and the mjml-react types.

no because it will break existing valid typescript implementations.

They're not valid though. DT types always follow the library types. We never intentionally deviate.

hmm. OK I'm not sure where to go from here.
The react binding types are out of sync with the component definition in mjml.
Merging this PR will result in a lot of errors for valid mjml.
I think we should fix this in the react bindings library.

I can open the PR to the mjml-react library tomorrow but it will probably take a few days to get it merged.

Can you wait for a couple more days to get this PR merged?

@eps1lon the PR upstream has merged. Are you able to loosen the types in this PR?

My expectation is that you shouldn't need to change any tests from the base branch for this change. Does that make sense or is there something I'm still missing about the test changes?

Updated types with wix-incubator/mjml-react#66

typescript-bot · 2021-10-14T19:48:56Z

@eps1lon One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

typescript-bot · 2021-11-05T16:21:44Z

@IanEdington Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

typescript-bot · 2021-11-08T02:39:07Z

@eps1lon: Everything looks good here. I am ready to merge this PR (at a4ed8d5) on your behalf whenever you think it's ready.

If you'd like that to happen, please post a comment saying:

Ready to merge

and I'll merge this PR almost instantly. Thanks for helping out! ❤️

(@henrinormak, @IanEdington: you can do this too.)

eps1lon · 2021-11-08T09:24:55Z

Ready to merge

typescript-bot added this to Needs Author Action in New Pull Request Status Board Oct 6, 2021

eps1lon force-pushed the fix/mjml-react/implcit-children branch from 26b264f to 846c753 Compare October 14, 2021 13:43

eps1lon marked this pull request as ready for review October 14, 2021 13:49

typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Oct 14, 2021

IanEdington mentioned this pull request Oct 14, 2021

[react] React 18 types #56210

Merged

6 tasks

IanEdington suggested changes Oct 14, 2021

View reviewed changes

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Oct 14, 2021

typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Oct 14, 2021

IanEdington mentioned this pull request Oct 29, 2021

Urgent: Adjust component prop-types for use in MjmlAttributes wix-incubator/mjml-react#66

Merged

typescript-bot removed the Revision needed This PR needs code changes before it can be merged. label Nov 5, 2021

typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Nov 5, 2021

typescript-bot added Edits Infrastructure Where is GH Actions? GH Actions didn't give a response to this PR labels Nov 5, 2021

typescript-bot moved this from Waiting for Code Reviews to Other in New Pull Request Status Board Nov 5, 2021

eps1lon added 4 commits November 5, 2021 17:16

Will only throw once we remove implicit children

c498255

Adjust children after propTypes changes

89bc1fc

Revert MjmlFont test changes

a4ed8d5

eps1lon force-pushed the fix/mjml-react/implcit-children branch from cfc5234 to a4ed8d5 Compare November 5, 2021 16:16

typescript-bot removed Edits Infrastructure Where is GH Actions? GH Actions didn't give a response to this PR labels Nov 5, 2021

typescript-bot moved this from Other to Waiting for Code Reviews in New Pull Request Status Board Nov 5, 2021

IanEdington approved these changes Nov 8, 2021

View reviewed changes

typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Nov 8, 2021

typescript-bot added the Self Merge This PR can now be self-merged by the PR author or an owner label Nov 8, 2021

typescript-bot moved this from Waiting for Code Reviews to Waiting for Author to Merge in New Pull Request Status Board Nov 8, 2021

typescript-bot moved this from Waiting for Author to Merge to Recently Merged in New Pull Request Status Board Nov 8, 2021

typescript-bot merged commit b5c13be into DefinitelyTyped:master Nov 8, 2021

typescript-bot removed this from Recently Merged in New Pull Request Status Board Nov 8, 2021

eps1lon deleted the fix/mjml-react/implcit-children branch November 21, 2021 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mjml-react] Add Explicit types for children #56254

[mjml-react] Add Explicit types for children #56254

eps1lon commented Oct 6, 2021 •

edited

typescript-bot commented Oct 14, 2021 •

edited

typescript-bot commented Oct 14, 2021

IanEdington commented Oct 14, 2021

eps1lon commented Oct 14, 2021

IanEdington Oct 14, 2021

eps1lon Oct 14, 2021

eps1lon Oct 27, 2021

IanEdington Oct 27, 2021

eps1lon Oct 27, 2021

IanEdington Oct 27, 2021

eps1lon Oct 27, 2021 •

edited

IanEdington Oct 28, 2021

IanEdington Nov 5, 2021 •

edited

eps1lon Nov 5, 2021

typescript-bot commented Oct 14, 2021

typescript-bot commented Nov 5, 2021

typescript-bot commented Nov 8, 2021

eps1lon commented Nov 8, 2021

[mjml-react] Add Explicit types for children #56254

[mjml-react] Add Explicit types for children #56254

Conversation

eps1lon commented Oct 6, 2021 • edited

typescript-bot commented Oct 14, 2021 • edited

1 package in this PR

Code Reviews

Status

typescript-bot commented Oct 14, 2021

IanEdington commented Oct 14, 2021

eps1lon commented Oct 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eps1lon Oct 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanEdington Nov 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typescript-bot commented Oct 14, 2021

typescript-bot commented Nov 5, 2021

typescript-bot commented Nov 8, 2021

eps1lon commented Nov 8, 2021

eps1lon commented Oct 6, 2021 •

edited

typescript-bot commented Oct 14, 2021 •

edited

eps1lon Oct 27, 2021 •

edited

IanEdington Nov 5, 2021 •

edited