Closed karlhorky closed 1 month ago
Seems interesting. Initial thoughts. 1.The extension should not be activated for MDX language unless they are considered as JSX (courtesy MDX extension)
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.
- Do yo see any error on hover ?
Yes, it's visible in the video
@karlhorky Did you notice this , ? the extension never gets activated like I presumed
@karlhorky I modified the langauge service plugin based on the volar PR
https://github.com/Viijay-Kr/typescript-cleanup-defs/pull/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
With "the Volar PR", you mean https://github.com/volarjs/volar.js/pull/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...
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.
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.
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?
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 https://github.com/Viijay-Kr/typescript-cleanup-defs/pull/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:
Hm, I just noticed new behavior / regression which appears to be caused by viijay-kr.react-ts-css@3.2.1
:
Does the same happen for you @Viijay-Kr ?
Interesting . Will look into it there has been new issue opened raising the same concern
Reopening because #164 reverted the fix in #161, because of this new regression:
@karlhorky The upstream issue https://github.com/mdx-js/mdx-analyzer/issues/451 has been closed . So having the latest version of mdx analyzer and react css modules does not solve the problem in this extension ?
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:
https://github.com/user-attachments/assets/ab2edde5-1827-40f7-9f74-606eb4a8cc48
@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 https://github.com/Viijay-Kr/typescript-cleanup-defs/commit/3e3e8cd2496fe4a6201d70fc6c8455c71591e055 , which was reverted), you may want to review the 2 Volar PRs from @johnsoncodehk for implementation hints:
@karlhorky Check this out https://github.com/Viijay-Kr/typescript-cleanup-defs/pull/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
Nice! I'll try to apply to my running extension and test out a first version
@Viijay-Kr thanks for the publish of viijay-kr.react-ts-css@3.2.4
, I can confirm the MDX errors are in the correct positions again! 🎉
https://github.com/user-attachments/assets/2e204850-b517-4873-962a-a65dafcf2ff4
Companion issue to https://github.com/mdx-js/mdx-analyzer/issues/451
Describe the bug
Usage with
unifiedjs.vscode-mdx@1.8.9
along withviijay-kr.react-ts-css@3.2.0
leads to:.mdx
files reported at end of fileZoomImage
adds the new import to the bottom of the codeDisabling
viijay-kr.react-ts-css@3.2.0
leads to errors being reported in the correct locationsTo Reproduce
Steps to reproduce the behavior:
package.json
dependenciesunifiedjs.vscode-mdx@1.8.9
andviijay-kr.react-ts-css@3.2.0
(both enabled)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 locationshttps://github.com/Viijay-Kr/react-ts-css/assets/1935696/6dc9d71b-7d75-4615-ba53-4a027eae83fc
Expected behavior
ZoomImage
should add the new import to the top of the codeScreenshots
Already included above
Desktop (please complete the following information):
VS Code version information:
Additional context
--