Base MDX type tests on real world examples #68955

remcohaszing · 2024-03-11T10:53:13Z

This bases the MDX type tests on real world examples. Different frameworks use different type definitions for the JSX namespace. This is what I envision the tests could look like.

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to reflect the change.
Follow the advice from the readme.
Avoid common mistakes.
Run pnpm test <package to test>.

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: https://discord.com/channels/508357248330760243/757992230077333655/1215735407824732170
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the package.json.

cc @jakebailey

This bases the MDX type tests on real world examples. Different frameworks use different type definitions for the JSX namespace.

jakebailey · 2024-03-11T16:13:49Z

ts-eslint's support for project references is not so good; you'd need to set this up just like I did #68908 and put tsconfigs without references at the root.

types/mdx/package.json

Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>

remcohaszing · 2024-03-11T17:00:04Z

types/mdx/preact-tests.tsx

+    namespace JSX {
+        type Element = runtime.JSX.Element;
+        type ElementClass = runtime.JSX.ElementClass;
+        type ElementType = runtime.JSX.ElementType;
+        type IntrinsicElements = runtime.JSX.IntrinsicElements;
+    }


I was a bit sad to find out this is necessary. I initially thought this would work, but it doesn’t:

declare module 'mdx/types' { import { JSX } from 'preact/jsx-runtime' }

jakebailey · 2024-03-11T17:01:29Z

I think to fix the current errors, you should go back to types: [] and instead use reference directives to load react/preact.

jakebailey · 2024-03-11T17:24:07Z

Seems like the test only fails due to the JSX namespace needing to be declared?

remcohaszing · 2024-03-12T10:40:39Z

The JSX namespace is declared in types/mdx/preact-tests.tsx via module augmentation. I don’t see why it’s currently not working. I’m pretty sure this worked before.

jakebailey · 2024-03-13T02:22:45Z

The one declared in the preact file is on the preact module, but index.d.ts is looking for the one in the globals.

jakebailey · 2024-03-14T16:18:05Z

Seems like it's passing; are you wanting this to get merged in? (It's currently a draft but seems good to me.)

remcohaszing · 2024-03-14T16:23:26Z

I think it’s good, but seeing Preact integration depends on declaration merging, I would like to add some docs for this. Also now that this works, I want to test Vue and Solid integration.

I’m marking it Ready for review to ping other maintainers, see if they also think this is a good idea.

typescript-bot · 2024-03-14T16:24:21Z

@remcohaszing Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

mdx — on npm, on unpkg (author is owner)
- owner-approval: @ChristianMurphy
- Config files to check:
  - mdx/tsconfig.preact.json: not the expected form (check: compilerOptions.jsxFactory)

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

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

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
✅ A DT maintainer needs to approve changes which affect module config files

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": 68955,
  "author": "remcohaszing",
  "headCommitOid": "d51924efb0e19162a72cc5cd33c738dae1ae6d8e",
  "mergeBaseOid": "55df51f723ed778b0960e1fe3ae43a3590a88d58",
  "lastPushDate": "2024-03-11T10:53:13.000Z",
  "lastActivityDate": "2024-03-22T10:52:38.000Z",
  "mergeOfferDate": "2024-03-22T00:06:29.000Z",
  "mergeRequestDate": "2024-03-22T10:52:38.000Z",
  "mergeRequestUser": "remcohaszing",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "mdx",
      "kind": "edit",
      "files": [
        {
          "path": "types/mdx/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/mdx/package.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/mdx/preact-tests.tsx",
          "kind": "test"
        },
        {
          "path": "types/mdx/react-tests.tsx",
          "kind": "test"
        },
        {
          "path": "types/mdx/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/mdx/tsconfig.preact.json",
          "kind": "package-meta",
          "suspect": "not [the expected form](https://github.com/DefinitelyTyped/DefinitelyTyped#user-content-tsconfigjson) (check: `compilerOptions.jsxFactory`)"
        },
        {
          "path": "types/mdx/tsconfig.react.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/mdx/types.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "ChristianMurphy",
        "remcohaszing",
        "wooorm"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "jakebailey",
      "date": "2024-03-22T00:05:46.000Z",
      "isMaintainer": true
    },
    {
      "type": "approved",
      "reviewer": "ChristianMurphy",
      "date": "2024-03-18T20:40:21.000Z",
      "isMaintainer": false
    }
  ],
  "mainBotCommentID": 1997845374,
  "ciResult": "pass"
}

typescript-bot · 2024-03-14T16:24:23Z

🔔 @ChristianMurphy @wooorm — 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.

typescript-bot · 2024-03-14T16:24:23Z

@remcohaszing Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

mdx — on npm, on unpkg (author is owner)
- owner-approval: @ChristianMurphy
- Config files to check:
  - mdx/tsconfig.preact.json: not the expected form (check: compilerOptions.jsxFactory)

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

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

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
✅ A DT maintainer needs to approve changes which affect module config files

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": 68955,
  "author": "remcohaszing",
  "headCommitOid": "d51924efb0e19162a72cc5cd33c738dae1ae6d8e",
  "mergeBaseOid": "55df51f723ed778b0960e1fe3ae43a3590a88d58",
  "lastPushDate": "2024-03-11T10:53:13.000Z",
  "lastActivityDate": "2024-03-22T10:52:38.000Z",
  "mergeOfferDate": "2024-03-22T00:06:29.000Z",
  "mergeRequestDate": "2024-03-22T10:52:38.000Z",
  "mergeRequestUser": "remcohaszing",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "mdx",
      "kind": "edit",
      "files": [
        {
          "path": "types/mdx/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/mdx/package.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/mdx/preact-tests.tsx",
          "kind": "test"
        },
        {
          "path": "types/mdx/react-tests.tsx",
          "kind": "test"
        },
        {
          "path": "types/mdx/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/mdx/tsconfig.preact.json",
          "kind": "package-meta",
          "suspect": "not [the expected form](https://github.com/DefinitelyTyped/DefinitelyTyped#user-content-tsconfigjson) (check: `compilerOptions.jsxFactory`)"
        },
        {
          "path": "types/mdx/tsconfig.react.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/mdx/types.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "ChristianMurphy",
        "remcohaszing",
        "wooorm"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "jakebailey",
      "date": "2024-03-22T00:05:46.000Z",
      "isMaintainer": true
    },
    {
      "type": "approved",
      "reviewer": "ChristianMurphy",
      "date": "2024-03-18T20:40:21.000Z",
      "isMaintainer": false
    }
  ],
  "mainBotCommentID": 1997845374,
  "ciResult": "pass"
}

typescript-bot · 2024-03-14T16:24:24Z

@jakebailey 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 · 2024-03-14T16:24:25Z

🔔 @ChristianMurphy @wooorm — 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.

typescript-bot · 2024-03-14T16:24:25Z

@jakebailey 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?

weswigham · 2024-03-14T16:53:08Z

I'm pretty sure if you set jsxImportSource to preact, the preact example won't need anything special, type-wise, at least for "jsx": "react-jsx"

remcohaszing · 2024-03-14T17:07:05Z

I'm pretty sure if you set jsxImportSource to preact, the preact example won't need anything special, type-wise, at least for "jsx": "react-jsx"

This used to be the case, but with frameworks moving away from the global JSX namespace, it no longer is.

Also DefinitelyTyped doesn’t allow specifying jsxImportSource. The classic runtime works for me so far, but maybe it should be allowed.

remcohaszing · 2024-03-14T17:10:11Z

I also tried this:

declare module "mdx/types" {
    import { JSX } from 'preact/jsx-runtime'
}

but that gave the type error Imports are not permitted in module augmentations. Consider moving them to the enclosing external module.ts(2667)

sheetalkamat · 2024-03-18T18:50:07Z

@jakebailey @sandersn @weswigham is this good to go or does this need some work as suggested by @weswigham in #68955 (comment) to support supplying jsxImportSource or similar ?

jakebailey · 2024-03-18T18:52:52Z

This PR is fine from my perspective, I believe we were just waiting for feedback from other MDX maintainers.

sheetalkamat

Marking this as change needed so that its not on maintainer board. please merge main when one of MDX maintainer approves so we can approve this.

typescript-bot · 2024-03-18T19:03:07Z

@remcohaszing 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!

jakebailey · 2024-03-18T19:10:15Z

FWIW you can just move this over to the "needs review" column and it'll "bless" the PR to say "maintainer said okay", I'm pretty sure.

ChristianMurphy

I don't have access to the discord that initiated this PR to have the context.
Reading between the lines a bit, it seems to focus on adding preact testing?

I'm all for supporting the types of other frameworks.
But don't use them enough to offer much constructive review on this PR.
I'll approved to unblock if others are comfortable.

/cc @wooorm who have more thoughts on this

ChristianMurphy · 2024-03-18T20:35:47Z

types/mdx/preact-tests.tsx

        img: CustomImageComponent,
        // @ts-expect-error
-        video: CustomImageComponent,
+        input: CustomImageComponent,


is there something about input that makes it a better test than video?

Yes, apparently input props are assignable to image props in Preact.

jakebailey · 2024-03-18T20:42:10Z

I don't have access to the discord that initiated this PR to have the context.
Reading between the lines a bit, it seems to focus on adding preact testing?

The context is that I added to DT the ability to check with multiple different tsconfigs, which means projects can check with different settings and references. This was mainly for node to check with/without DOM types, but can also be used for MDX to test against react, preact, etc.

ChristianMurphy · 2024-03-18T21:14:36Z

Thanks for the context @jakebailey! That helps

outdated

jakebailey

Doesn't seem like there's going to be other feedback; going to approve this and you can feel free to merge if you believe that it's all good.

typescript-bot · 2024-03-22T00:06:29Z

@remcohaszing: Everything looks good here. I am ready to merge this PR (at d51924e) 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! ❤️

(@ChristianMurphy, @wooorm: you can do this too.)

remcohaszing · 2024-03-22T10:52:32Z

It looks like both Solid and Vue only work with the automatic runtime types, which are not allowed by dtslint. I’ll merge as-is.

remcohaszing · 2024-03-22T10:52:38Z

Ready to merge

Base MDX type tests on real worls examples

e5c0417

This bases the MDX type tests on real world examples. Different frameworks use different type definitions for the JSX namespace.

remcohaszing added 2 commits March 11, 2024 17:30

Refactor MDX setup

936413d

Fix some copy-paste errors

c7666ca

remcohaszing force-pushed the mdx-test-real-frameworks branch from 153524a to c7666ca Compare March 11, 2024 16:36

jakebailey reviewed Mar 11, 2024

View reviewed changes

types/mdx/package.json Show resolved Hide resolved

Restore framework-less MDX tests

d0d8144

jakebailey reviewed Mar 11, 2024

View reviewed changes

types/mdx/package.json Outdated Show resolved Hide resolved

Update MDX tsconfigs

b69a78b

jakebailey reviewed Mar 11, 2024

View reviewed changes

types/mdx/package.json Outdated Show resolved Hide resolved

Update types/mdx/package.json

8c86bd0

Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>

remcohaszing commented Mar 11, 2024

View reviewed changes

remcohaszing added 2 commits March 11, 2024 18:05

Remove frameworkless tests

178f8b9

Remove redundant types from mdx

de6df32

jakebailey mentioned this pull request Mar 11, 2024

[node] Remove ts4.8 in favor of new multi-tsconfig testing #68908

Merged

remcohaszing changed the title ~~Base MDX type tests on real worls examples~~ Base MDX type tests on real world examples Mar 12, 2024

Fix the use of JSX.Element in mdx index.d.ts

d51924e

remcohaszing marked this pull request as ready for review March 14, 2024 16:23

typescript-bot added Critical package Author is Owner The author of this PR is a listed owner of the package. Check Config Changes a module config files labels Mar 14, 2024

sheetalkamat previously requested changes Mar 18, 2024

View reviewed changes

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Mar 18, 2024

ChristianMurphy approved these changes Mar 18, 2024

View reviewed changes

jakebailey approved these changes Mar 22, 2024

View reviewed changes

typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner and removed Revision needed This PR needs code changes before it can be merged. labels Mar 22, 2024

typescript-bot merged commit a49cfed into DefinitelyTyped:master Mar 22, 2024

remcohaszing deleted the mdx-test-real-frameworks branch March 22, 2024 10:53

remcohaszing mentioned this pull request Mar 22, 2024

Allow the jsxImportSource compiler option microsoft/DefinitelyTyped-tools#982

Merged

Base MDX type tests on real world examples #68955

Base MDX type tests on real world examples #68955

Uh oh!

Conversation

remcohaszing commented Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakebailey commented Mar 11, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

remcohaszing Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

jakebailey commented Mar 11, 2024

Uh oh!

jakebailey commented Mar 11, 2024

Uh oh!

remcohaszing commented Mar 12, 2024

Uh oh!

jakebailey commented Mar 13, 2024

Uh oh!

jakebailey commented Mar 14, 2024

Uh oh!

remcohaszing commented Mar 14, 2024

Uh oh!

typescript-bot commented Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1 package in this PR

Code Reviews

Status

Uh oh!

typescript-bot commented Mar 14, 2024

Uh oh!

typescript-bot commented Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1 package in this PR

Code Reviews

Status

Uh oh!

typescript-bot commented Mar 14, 2024

Uh oh!

typescript-bot commented Mar 14, 2024

Uh oh!

typescript-bot commented Mar 14, 2024

Uh oh!

weswigham commented Mar 14, 2024

Uh oh!

remcohaszing commented Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

remcohaszing commented Mar 14, 2024

Uh oh!

sheetalkamat commented Mar 18, 2024

Uh oh!

jakebailey commented Mar 18, 2024

Uh oh!

sheetalkamat left a comment

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Mar 18, 2024

Uh oh!

jakebailey commented Mar 18, 2024

Uh oh!

ChristianMurphy left a comment

Choose a reason for hiding this comment

Uh oh!

ChristianMurphy Mar 18, 2024

Choose a reason for hiding this comment

Uh oh!

remcohaszing Mar 22, 2024

Choose a reason for hiding this comment

Uh oh!

jakebailey commented Mar 18, 2024

Uh oh!

ChristianMurphy commented Mar 18, 2024

Uh oh!

jakebailey left a comment

Choose a reason for hiding this comment

remcohaszing commented Mar 11, 2024 •

edited

Loading

typescript-bot commented Mar 14, 2024 •

edited

Loading

typescript-bot commented Mar 14, 2024 •

edited

Loading

remcohaszing commented Mar 14, 2024 •

edited

Loading