Errors in .mdx files with mdx.checkMdx in tsconfig.json appear at end of file

[karlhorky]

Companion issue to mdx-js/mdx-analyzer#451

Describe the bug

Usage with [email protected] along with [email protected] leads to:

1. All errors in .mdx files reported at end of file
2. Selecting the IntelliSense auto-import entry for ZoomImage adds the new import to the bottom of the code

Disabling [email protected] leads to errors being reported in the correct locations

To Reproduce

Steps to reproduce the behavior:

1. Clone the reproduction at https://github.com/karlhorky/repro-mdx-analyzer-missing-auto-import-intellisense
2. Install the package.json dependencies
3. Install the 2 recommended extensions [email protected] and [email protected] (both enabled)
4. In app/message.mdx, observe that the errors are reported at a one-character red squiggly at the end of the file, instead of in the correct locations

Kapture.2024-06-28.at.17.33.21.mp4

Expected behavior

1. Error reported in correct location
2. Selecting the IntelliSense auto-import entry for ZoomImage should add the new import to the top of the code

Screenshots

Already included above

Desktop (please complete the following information):

• OS: macOS
• Version Sonoma 14.5 (23F79)

VS Code version information:

Version: 1.91.0-insider
Commit: aea213b7fcc7de5c24ad797ac1af209b159d451f
Date: 2024-06-28T06:02:58.030Z
Electron: 29.4.0
ElectronBuildId: 9728852
Chromium: 122.0.6261.156
Node.js: 20.9.0
V8: 12.2.281.27-electron.0
OS: Darwin arm64 23.5.0

Additional context

--

[Viijay-Kr]

Seems interesting. Initial thoughts. 1.The extension should not be activated for MDX language unless they are considered as JSX (courtesy MDX extension)
2. Do yo see any error on hover ?

[karlhorky]

1.The extension should not be activated for MDX language unless they are considered as JSX (courtesy MDX extension)

Good to know, maybe that's useful information for tracking down this problem.

2. Do yo see any error on hover ?

Yes, it's visible in the video

[Viijay-Kr]

@karlhorky Did you notice this , ? the extension never gets activated like I presumed

[Viijay-Kr]

@karlhorky I modified the langauge service plugin based on the volar PR

Viijay-Kr/typescript-cleanup-definitions#4 .

This fixes the problem for me . I tested it locally . If you wish to test it ? clone the repo , link the package in react-ts-css project and run the extension locally , the errors appear at the right location

[karlhorky]

With "the Volar PR", you mean volarjs/volar.js#216, right?

I wonder if this PR will actually resolve the issue, once it is pulled in to MDX Analyzer and a new version of MDX Analyzer released...

This fixes the problem for me . I tested it locally . If you wish to test it ? clone the repo , link the package in react-ts-css project and run the extension locally , the errors appear at the right location

Sounds great though! Maybe it's best to have it resolved in both extensions anyway. I will try to find some time in the next days to test this...

[Viijay-Kr]

I wonder if this PR will actually resolve the issue, once it is pulled in to MDX Analyzer and a new version of MDX Analyzer released...

I don't follow this 🤔. I haven't gone over mdx analyzer project. Does MDX analyzer process all the typrescript server plugins installed by other extensions ? even if it did I tested it using the setup you recommended , using the mdx analyzer extension. So please enlighten me If I am missing something

With "the Volar PR", you mean volarjs/volar.js#216, right?

Yes thats right.

[karlhorky]

I don't follow this 🤔. I haven't gone over mdx analyzer project. Does MDX analyzer process all the typrescript server plugins installed by other extensions ? even if it did I tested it using the setup you recommended , using the mdx analyzer extension. So please enlighten me If I am missing something

Ah I'm not sure you're missing anything - I was just wondering if fixing it in MDX Analyzer would also fix it in other extensions. But fixing in both places (also in React CSS modules) is probably better and maybe also even necessary.

[karlhorky]

I tested it locally . If you wish to test it ? clone the repo , link the package in react-ts-css project and run the extension locally , the errors appear at the right location

I would try to test this out today - just having trouble understanding the instructions.

Is there a way to apply the patch manually (eg. from the Files changed in the PR) by editing the extension files locally on my machine? (instead of building/linking anything or installing a different local version of the extension) Something similar to the workflow of editing node_modules by hand?

[karlhorky]

Ah yeah, I can edit the files in place by editing this file:

code ~/.vscode/extensions/viijay-kr.react-ts-css-3.2.0/node_modules/typescript-cleanup-definitions/index.js

This was the change I applied (based on Viijay-Kr/typescript-cleanup-definitions#4):

-            const proxy = Object.create(null);
+            const proxy = new Proxy(info.languageService, {
+                get(target, p, receiver) {
+                    return Reflect.get(target, p, receiver)
+                },
+                set(target, p, newValue, receiver) {
+                    return Reflect.set(target, p, newValue, receiver)
+                },
+            });

This indeed resolves the problem:

Before

After

[karlhorky]

Hm, I just noticed new behavior / regression which appears to be caused by [email protected]:

• cmd-click no longer opens the peek view or jumps to definitions (also in TypeScript files)

Does the same happen for you @Viijay-Kr ?

[Viijay-Kr]

Interesting . Will look into it there has been new issue opened raising the same concern

[karlhorky]

Reopening because #164 reverted the fix in #161, because of this new regression:

• Breaks Go to Definition for some reason #163

[Viijay-Kr]

@karlhorky The upstream issue mdx-js/mdx-analyzer#451 has been closed . So having the latest version of mdx analyzer and react css modules does not solve the problem in this extension ?

[karlhorky]

Copying my comment here for visibility:

The problem is half fixed: I tried out the new version with the reproduction repo from the original issue description and:

1. Correct error locations are used with the (deprecated) Styled Components VS Code extension
2. Incorrect error locations (end of file) still appear with the React CSS modules VS Code extension (related issue: Errors in .mdx files with mdx.checkMdx in tsconfig.json appear at end of file #160)

Kapture.2024-08-20.at.10.03.57.mp4

[karlhorky]

@Viijay-Kr in case you want to revisit implementing a fix on the side of the React CSS modules extension (potentially in a style similar to the original fix Viijay-Kr/typescript-cleanup-definitions@3e3e8cd , which was reverted), you may want to review the 2 Volar PRs from @johnsoncodehk for implementation hints:

• fix(typescript): synchronize script snapshots in a side-effect-free manner volarjs/volar.js#226
• fix(typescript): use Proxy instead of modifying LanguageService instance volarjs/volar.js#216

[Viijay-Kr]

@karlhorky Check this out
Viijay-Kr/typescript-cleanup-definitions#5

I have adapted the changes similar to volar.
Turns out in previous MR I did not do the changes as intended. A bit premature .

I tested this one, so prior definitions work as expected and also fiters out unnecessary definitions according to our projects expectation.

So it should be a potential fix

[karlhorky]

Nice! I'll try to apply to my running extension and test out a first version

[karlhorky]

@Viijay-Kr thanks for the publish of [email protected], I can confirm the MDX errors are in the correct positions again! 🎉

Kapture.2024-08-22.at.20.20.01.mp4