Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Errors in .mdx files with mdx.checkMdx appear at end of file #451

Closed
4 tasks done
karlhorky opened this issue Jun 27, 2024 · 20 comments · Fixed by #460
Closed
4 tasks done

Errors in .mdx files with mdx.checkMdx appear at end of file #451

karlhorky opened this issue Jun 27, 2024 · 20 comments · Fixed by #460
Labels
🗄 area/interface This affects the public interface 🌊 blocked/upstream This cannot progress before something external happens first 👍 phase/yes Post is accepted and can be worked on 🦋 type/enhancement This is great to have 🙆 yes/confirmed This is confirmed and ready to be worked on

Comments

@karlhorky
Copy link

karlhorky commented Jun 27, 2024

Initial checklist

Affected packages and versions

MDX VS Code Extension v1.8.9

Link to runnable example

No response

Steps to reproduce

Reproduction repo: https://github.com/karlhorky/repro-mdx-analyzer-missing-auto-import-intellisense

  1. Set up the app-dir-mdx example from Remco using the content below

(content contains intentional TypeScript errors)

mdx-components.tsx

import { MDXComponents } from 'mdx/types';
import { HTMLAttributes } from 'react';

type ZoomImageProps = {
  alt: string;
};

function ZoomImage(props: ZoomImageProps) {
  return <div>www</div>;
}

function CustomCodeSandBox(props: HTMLAttributes<HTMLDivElement>) {
  return (
    <div {...props} />
  );
}

const components = {
  ZoomImage,
  CustomCodeSandBox,
} satisfies MDXComponents;

declare global {
  type MDXProvidedComponents = typeof components;
}

export function useMDXComponents(): MDXProvidedComponents {
  return components;
}

app/message.mdx

<ZoomImage src="/logo.svg" alt="Logo" />

<CustomCodeSandBox template="react" />

tsconfig.json

{
  "compilerOptions": {
    "target": "es5",
    "lib": ["dom", "dom.iterable", "esnext"],
    "allowJs": true,
    "skipLibCheck": true,
    "strict": true,
    "forceConsistentCasingInFileNames": true,
    "noEmit": true,
    "esModuleInterop": true,
    "module": "esnext",
    "moduleResolution": "node",
    "resolveJsonModule": true,
    "isolatedModules": true,
    "jsx": "preserve",
    "incremental": true,
    "plugins": [
      {
        "name": "next"
      }
    ]
  },
  "mdx": {
    "checkMdx": true
  },
  "include": [
    "next-env.d.ts",
    "**/*.mdx",
    "**/*.ts",
    "**/*.tsx",
    ".next/types/**/*.ts"
  ],
  "exclude": ["node_modules"]
}
  1. Observe the errors from the MDX VS Code extension appearing on a 1-character red squiggly the end of the file in VS Code instead of on the respective lines where the errors occur:
    Screenshot 2024-06-27 at 19 48 42

Expected behavior

In the screenshot above, I would expect that the 1st error would appear on line 47, covering the whole line, and the 2nd error would appear on line 49, covering the whole line

Actual behavior

In the screenshot above, the errors from the MDX VS Code extension appear on a 1-character red squiggly the end of the file in VS Code instead of on the respective lines where the errors occur

Runtime

No response

Package manager

No response

OS

macOS

Build and bundle tools

Next.js

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Jun 27, 2024
@karlhorky
Copy link
Author

karlhorky commented Jun 27, 2024

Interesting, it appears that the highlighting is in the correct location if I disable these 2 extensions 🤔

Screenshot 2024-06-27 at 23 06 27

@karlhorky
Copy link
Author

karlhorky commented Jun 28, 2024

Funny enough, if I disable these 2 extensions, then that also disables the IntelliSense to auto-import into the file:

Extensions enabled

Screenshot 2024-06-27 at 23 07 38

Screenshot 2024-06-28 at 10 45 42

(although it is weird that the import appears at the end of the file... I guess it could be related to the error being reported at the end though)

Extensions disabled

Screenshot 2024-06-27 at 23 07 30

...or maybe IntelliSense to auto-import is not a feature of MDX Analyzer? 🤔


There is also another IntelliSense issue below with vscode-styled-components

However this appears to exhibit the opposite behavior - according to users in 413, enabling these extensions apparently caused IntelliSense suggestions to disappear on older versions (whereas I'm seeing IntelliSense suggestions only appear when the extensions are enabled):

@remcohaszing
Copy link
Member

I’m closing this in favour of upstream issue styled-components/vscode-styled-components#448.

I’m not seeing any problems while using viijay-kr.react-ts-css.

@remcohaszing remcohaszing closed this as not planned Won't fix, can't repro, duplicate, stale Jun 28, 2024

This comment has been minimized.

@remcohaszing remcohaszing added the 👀 no/external This makes more sense somewhere else label Jun 28, 2024
Copy link
Contributor

github-actions bot commented Jun 28, 2024

Hi team! Could you describe why this has been marked as external?

Thanks,
— bb

@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 🤞 phase/open Post is being triaged manually labels Jun 28, 2024
@karlhorky
Copy link
Author

karlhorky commented Jun 28, 2024

Ok, thanks for reporting!

I’m not seeing any problems while using viijay-kr.react-ts-css.

I think this may be only caused by certain settings in React CSS modules by @Viijay-Kr (viijay-kr.react-ts-css)

eg. here are my settings:

  "reactTsScss.autoComplete": true,
  "reactTsScss.baseDir": "src",
  "reactTsScss.cleanUpDefs": [
    "node_modules/vite/client.d.ts",
    "node_modules/next/types/global.d.ts",
    "node_modules/react-scripts/lib/react-app.d.ts"
  ],
  "reactTsScss.codelens": true,
  "reactTsScss.cssAutoComplete": true,
  "reactTsScss.cssDefinitions": true,
  "reactTsScss.cssSyntaxColor": true,
  "reactTsScss.definition": true,
  "reactTsScss.diagnostics": true,
  "reactTsScss.peekProperties": true,
  "reactTsScss.references": true,
Kapture.2024-06-28.at.15.48.48.mp4

@remcohaszing
Copy link
Member

I’m just not seeing it.

image

Possibly another extension is the culprit. All TypeScript plugins interact with each other and can accidentally break other plugins. I even recall one plugin recovered issues from another at some point. Perhaps another extension is the culprit.

The following command lists all VSCode extensions that register a TypeScript plugin.

grep -l '"typescriptServerPlugins"' ~/.vscode/extensions/*/package.json

@karlhorky
Copy link
Author

Hm, I can repro with only unifiedjs.vscode-mdx@1.8.9 and viijay-kr.react-ts-css@3.2.0

I'll create a reproduction.

@remcohaszing
Copy link
Member

FWIW this is how I’m testing this:

  • git clone git@github.com:mdx-js/mdx-analyzer.git
    cd mdx-analyzer
    npm install
    code .
  • Tweak fixtures/fixtures.code-workspace as desired.
  • In VSCode, press F5

@karlhorky
Copy link
Author

karlhorky commented Jun 28, 2024

Here's the reproduction (also shows the missing auto-import IntelliSense - I opened #452 for that):

Extensions: unifiedjs.vscode-mdx@1.8.9 and viijay-kr.react-ts-css@3.2.0

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

Repo: https://github.com/karlhorky/repro-mdx-analyzer-missing-auto-import-intellisense

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

@karlhorky
Copy link
Author

Interesting, it appears that the highlighting is in the correct location if I disable these 2 extensions 🤔

Companion issues have been reported in these two VS Code extensions now:

@remcohaszing
Copy link
Member

I’m re-opening this. A change was made upstream in volarjs/volar.js#216, which fixes compatibility with a number of existing TypeScript plugins. Let’s track here until that change has made its way into MDX analyzer.

@remcohaszing remcohaszing reopened this Jun 30, 2024
@github-actions github-actions bot removed 👎 phase/no Post cannot or will not be acted on 👀 no/external This makes more sense somewhere else labels Jun 30, 2024

This comment has been minimized.

@remcohaszing remcohaszing added 🦋 type/enhancement This is great to have 🗄 area/interface This affects the public interface 🌊 blocked/upstream This cannot progress before something external happens first labels Jun 30, 2024
@remcohaszing remcohaszing added 🙆 yes/confirmed This is confirmed and ready to be worked on 👍 phase/yes Post is accepted and can be worked on labels Jun 30, 2024
Copy link
Contributor

Hi! This was marked as ready to be worked on! Note that while this is ready to be worked on, nothing is said about priority: it may take a while for this to be solved.

Is this something you can and want to work on?

Team: please use the area/* (to describe the scope of the change), platform/* (if this is related to a specific one), and semver/* and type/* labels to annotate this. If this is first-timers friendly, add good first issue and if this could use help, add help wanted.

@jasonwilliams
Copy link

Thanks for raising the detailed issue, sadly I've had to archive https://github.com/styled-components/vscode-styled-components due to having no contributors for quite some time. It's unlikely the issue will be fixed there any time soon, I don't know if there's a replacement extension but until then it will remain the way it is.

@karlhorky
Copy link
Author

karlhorky commented Jul 6, 2024

I’m re-opening this. A change was made upstream in volarjs/volar.js#216, which fixes compatibility with a number of existing TypeScript plugins. Let’s track here until that change has made its way into MDX analyzer.

@remcohaszing Nice, thanks!

For anyone following along, considering that volarjs/volar.js#216 was released in v2.4.0-alpha.1, I guess the version that will probably have the change will be v2.4.0:

Once that version has been released, it can be bumped in MDX Analyzer too.

Copy link
Contributor

Hi! This was closed. Team: If this was fixed, please add phase/solved. Otherwise, please add one of the no/* labels.

@karlhorky
Copy link
Author

karlhorky commented Aug 20, 2024

@johnsoncodehk @remcohaszing Thanks for the update to Volar 2.4 in PR #460 and the publish of unifiedjs.vscode-mdx@1.8.10 🙌

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 Viijay-Kr/react-ts-css#160)
Kapture.2024-08-20.at.10.03.57.mp4

@Viijay-Kr
Copy link

Viijay-Kr commented Aug 20, 2024

@karlhorky Re posting here for visibility

This PR Viijay-Kr/typescript-cleanup-definitions#5 adapts similar changes done in volar project . Hopefully this fixes the issue fully

@karlhorky
Copy link
Author

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 🌊 blocked/upstream This cannot progress before something external happens first 👍 phase/yes Post is accepted and can be worked on 🦋 type/enhancement This is great to have 🙆 yes/confirmed This is confirmed and ready to be worked on
Development

Successfully merging a pull request may close this issue.

4 participants