Add js extension to mdx/types import by Gerrit0 · Pull Request #2211 · mdx-js/mdx

Gerrit0 · 2022-12-31T16:01:57Z

Because @mdx-js/mdx's package.json includes "type": "module", imports within it which reference a file must include file extensions, including type only imports. This breaks downstream TypeScript users who have "module": "Node16" in their tsconfig.json file.

node_modules/@mdx-js/mdx/lib/evaluate.d.ts:23:32 - error TS2307: Cannot find module 'mdx/types' 
  or its corresponding type declarations.

23 export type ExportMap = import('mdx/types').MDXModule;
                                  ~~~~~~~~~~~

Yes, the .js looks odd since we're importing a declaration file from @types/mdx here, but it is correct, since in a TS-native setup, it would be describing a .js file alongside the declaration file.

cc: @Gaurishh

vercel · 2022-12-31T16:02:01Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
mdx	✅ Ready (Inspect)	Visit Preview	Jan 2, 2023 at 4:33PM (UTC)

codecov-commenter · 2022-12-31T16:05:23Z

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (88ae939) compared to base (600b12a).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##              main     #2211   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         2064      2064           
=========================================
  Hits          2064      2064

Impacted Files	Coverage Δ
packages/mdx/lib/evaluate.js	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

remcohaszing

Thanks! Nice catch!

ChristianMurphy

LGTM, thanks!
Would it also make sense to set the module type in the root configuration, so this is tested going forward?

mdx/tsconfig.json

Lines 2 to 6 in 600b12a

    
           "compilerOptions": { 
        
             "target": "ES2020", 
        
             "lib": ["ES2020"], 
        
             "module": "ES2020", 
        
             "moduleResolution": "node",

remcohaszing · 2023-01-01T13:57:48Z

Adding on @ChristianMurphy’s comment, the correct configuration would be to replace the module value with node16 and remove the moduleResolution and allowSyntheticDefaultImports options.

I also wouldn’t mind removing the redundant allowJs option and adding "forceConsistentCasingInFileNames": true and "newLine": "lf" while we’re fixing up tsconfig.json.

wooorm · 2023-01-02T09:06:55Z

I agree that module: node16 is a good idea.

Other refactoring of tsconfig should be a separate PR tho

Gerrit0 · 2023-01-02T16:01:56Z

... yeah, thought that might happen. Main reason I spot fixed the one piece breaking external consumers.

ChristianMurphy · 2023-01-02T16:17:15Z

At least 15 of the errors are in the lib/ folder of their respective project, the project implementation, and likely do impact downstream users.

Gerrit0 · 2023-01-02T16:21:59Z

... okay, so that wasn't some weird local issue... thought I just didn't know how to use workspaces

ChristianMurphy · 2023-01-02T16:35:35Z

The estree-walker error is likely related to or the same as Rich-Harris/estree-walker#31

Gerrit0 · 2023-01-02T16:37:51Z

I think I'll leave this here - don't have the time to spend a ton of time troubleshooting in an unfamiliar repo I don't use ;)

ChristianMurphy · 2023-01-02T16:46:08Z

unfamiliar repo I don't use

The errors are coming out of @mdx-js/mdx the one and only part of the repo you explicitly said you do use and originally edited. 🙂

That said, with estree-walker and periscopic being blockers, it likely makes sense to walk node16 back for a bit here, and nudge @Rich-Harris with some PRs to round out and fix ESM support upstream.

Gerrit0 · 2023-01-02T17:23:28Z

This PR is coming out of a question on the TypeScript Discord - I don't actually use @mdx-js/mdx, just helped someone else get around these errors then figured it should be easy to fix them upstream.

I agree - the Node16 changes would be nice, but until ESM works properly for those dependencies...

remcohaszing · 2023-01-02T19:38:26Z

That said, with estree-walker and periscopic being blockers, it likely makes sense to walk node16 back for a bit here,

Yes, but this shouldn’t block this PR. Even if this doesn’t test it, it does fix the issue.

An alternative solution could be to add exports in @types/mdx.

wooorm · 2023-01-02T19:46:37Z

What is blocking? I don’t see CI breaking?

remcohaszing · 2023-01-02T19:53:52Z

Potentially the fact that we can't use and therefor can't test using module: node16. If I misinterpreted that, then never mind. 🙂

Regardless, it's still worth considering the alternative. I currently don't have a strong opinion about it.

Gerrit0 · 2023-01-02T19:57:34Z

the fact that we can't use and therefor can't test using module: node16

Yep, exactly

wooorm · 2023-01-02T19:59:09Z

Oh wait, I missed that, I thought it was added here.
If estree-walker is failing, why not add @ts-expect-error for it? One of the benefits of only using TS for types, not for compiling. I did that here: syntax-tree/estree-util-build-jsx@3c78c80 (L26)

Gerrit0 · 2023-01-02T20:35:24Z

Unfortunately doing that would result in two modules which currently have intellisense being stuck with any, which I suspect would be an overall worse developer experience here.

https://github.com/mdx-js/mdx/actions/runs/3823843079/jobs/6505439634#step:5:25

wooorm · 2023-01-02T20:54:04Z

Internal types is fine, we have tests. They don’t reach users of this project.

You should be able to define anys as things, e.g., like this: /** @type {Node} */.
You can also ignore anys with:

// Types of `estree-walker` are wrong
// type-coverage:ignore-next-line

remcohaszing · 2023-01-02T21:04:20Z

We could also use patch-package to patch estree-walker, since that's already used anyway.

Rich-Harris · 2023-01-03T20:07:10Z

I merged Rich-Harris/estree-walker#31 and released estree-walker@3.0.2, is there anything else I need to fix?

wooorm · 2023-01-03T20:16:47Z

Nice, I don’t think so! Thanks Rich!

(while you’re here, there was another bug with export maps in periscopic/estree-walker/is-reference, it’s now fixed in Next.js/SWC, but might pop up in other compilers: #2168 (reply in thread)). TLDR: Node recommends always adding a default condition, which isn’t (wasn’t?) in these packages!

ChristianMurphy · 2023-01-03T20:59:08Z

is there anything else I need to fix?

Thanks @Rich-Harris! Much appreciated!
Potentially a few more tweaks could be merged for the typings to support the new node16 resolution.

Rich-Harris/is-reference#14
Rich-Harris/periscopic#19

and one incoming for extree-walker as well

Closes GH-2211. Closes GH-2228.

wooorm · 2023-02-08T16:25:16Z

Done! :)

Add js extension to mdx/types import

01a6cbd

vercel bot deployed to Preview December 31, 2022 16:03 View deployment

remcohaszing approved these changes Dec 31, 2022

View reviewed changes

ChristianMurphy approved these changes Dec 31, 2022

View reviewed changes

Update module to Node16

e6c7161

vercel bot deployed to Preview January 2, 2023 16:01 View deployment

Add .js extension to remaining imports

547ef5a

vercel bot deployed to Preview January 2, 2023 16:20 View deployment

Revert tsconfig.json changes

88ae939

vercel bot deployed to Preview January 2, 2023 16:33 View deployment

ChristianMurphy mentioned this pull request Feb 8, 2023

Add module: node16 to tsconfig.json #2228

Merged

wooorm closed this in #2228 Feb 8, 2023

wooorm pushed a commit that referenced this pull request Feb 8, 2023

Add module: node16 to tsconfig.json

3911148

Closes GH-2211. Closes GH-2228.

wooorm added ☂️ area/types This affects typings 💪 phase/solved Post is done labels Feb 8, 2023

	"compilerOptions": {
	"target": "ES2020",
	"lib": ["ES2020"],
	"module": "ES2020",
	"moduleResolution": "node",

Uh oh!

Conversation

Gerrit0 commented Dec 31, 2022

Uh oh!

vercel bot commented Dec 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Dec 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

remcohaszing left a comment

Choose a reason for hiding this comment

Uh oh!

ChristianMurphy left a comment

Choose a reason for hiding this comment

Uh oh!

remcohaszing commented Jan 1, 2023

Uh oh!

wooorm commented Jan 2, 2023

Uh oh!

Gerrit0 commented Jan 2, 2023

Uh oh!

ChristianMurphy commented Jan 2, 2023

Uh oh!

Gerrit0 commented Jan 2, 2023

Uh oh!

ChristianMurphy commented Jan 2, 2023

Uh oh!

Gerrit0 commented Jan 2, 2023

Uh oh!

ChristianMurphy commented Jan 2, 2023

Uh oh!

Gerrit0 commented Jan 2, 2023

Uh oh!

remcohaszing commented Jan 2, 2023

Uh oh!

wooorm commented Jan 2, 2023

Uh oh!

remcohaszing commented Jan 2, 2023

Uh oh!

Gerrit0 commented Jan 2, 2023

Uh oh!

wooorm commented Jan 2, 2023

Uh oh!

Gerrit0 commented Jan 2, 2023

Uh oh!

wooorm commented Jan 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

remcohaszing commented Jan 2, 2023

Uh oh!

Rich-Harris commented Jan 3, 2023

Uh oh!

wooorm commented Jan 3, 2023

Uh oh!

ChristianMurphy commented Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wooorm commented Feb 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

vercel bot commented Dec 31, 2022 •

edited

Loading

codecov-commenter commented Dec 31, 2022 •

edited

Loading

wooorm commented Jan 2, 2023 •

edited

Loading

ChristianMurphy commented Jan 3, 2023 •

edited

Loading