Skip to content

Conversation

@Jinjiang
Copy link
Member

@Jinjiang Jinjiang commented Dec 3, 2025

Proposed Changes

@Jinjiang Jinjiang force-pushed the jinjiang/mdx-update-20251202 branch from 25ffdd0 to a443eab Compare December 3, 2025 04:52
Copilot AI review requested due to automatic review settings December 4, 2025 09:29
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates all MDX dependencies and usage from v1 to v3, modernizing the MDX compilation pipeline across the codebase.

  • Updates @mdx-js/mdx and @mdx-js/loader to v3.1.1
  • Replaces custom MDX transpiler with native MDX v3 APIs (compileSync)
  • Updates webpack loaders and configurations to use MDX v3 loader chain

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
workspace.jsonc Updates MDX packages to v3.1.1, adds new mdx-v3-options module, removes deprecated mdx-transpiler and mdx-loader components
scripts/find-unused-packages.js Removes @mdx-js/mdx from whitelist since it's now directly imported
scopes/react/react/webpack/webpack.config.component.dev.ts Replaces custom mdx-loader with @mdx-js/loader v3 and adds mdx-pre-loader
scopes/react/react/webpack/webpack.config.base.ts Updates MDX loader configuration to use v3 loader with mdx-v3-options
scopes/mdx/mdx/mdx.doc-reader.ts Migrates from custom compile API to native compileSync with updated metadata access pattern
scopes/mdx/mdx/mdx.detector.ts Updates to use native MDX v3 compileSync API and defines ImportSpecifier type locally
scopes/mdx/mdx/mdx.compiler.ts Switches to native MDX v3 compileSync and uses toString() instead of .contents property
components/legacy/e2e-helper/excluded-fixtures/workspace-with-tsconfig-issue/workspace.jsonc Updates test fixture dependency versions to match MDX v3 upgrade
.circleci/config.yml Updates cache keys to reflect new core-aspect-env version (v0.1.0)
.bitmap Adds environment configuration to all components specifying their env versions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

"@mdx-js/mdx": "1.6.21",
"@mdx-js/loader": "^3.1.1",
"@mdx-js/mdx": "^3.1.1",
"@mdx-js/react": "1.6.22",
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The @mdx-js/react package version is still at 1.6.22 (MDX v1), which is inconsistent with the MDX v3 upgrade. This package should be updated to a v3-compatible version (e.g., ^3.1.1) to ensure compatibility with @mdx-js/mdx and @mdx-js/loader which are now at v3.

Suggested change
"@mdx-js/react": "1.6.22",
"@mdx-js/react": "^3.1.1",

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings December 4, 2025 10:26
@Jinjiang Jinjiang force-pushed the jinjiang/mdx-update-20251202 branch from db398fd to c059a51 Compare December 4, 2025 10:26
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}
}
`;
` as any;
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The addition of as any type assertions to GraphQL queries appears to be a workaround for a type compatibility issue introduced by upgrading Apollo Client from an unspecified version to ^3.6.0. While this resolves immediate type errors, it bypasses TypeScript's type checking for GraphQL queries and mutations. Consider using proper TypeScript codegen for GraphQL operations (e.g., @graphql-codegen/cli) or updating to a more recent Apollo Client version that has better TypeScript support, rather than using as any assertions.

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings December 4, 2025 12:44
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 74 to 80
{
loader: require.resolve('@teambit/mdx.modules.mdx-loader'),
loader: require.resolve('@mdx-js/loader'),
options: mdxOptions,
},
{
loader: require.resolve('@teambit/mdx.modules.mdx-pre-loader'),
},
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect loader order for MDX processing

Webpack loaders execute from bottom to top in the use array. The current order has:

  1. @mdx-js/loader (executed second)
  2. @teambit/mdx.modules.mdx-pre-loader (executed first)

This means the pre-loader runs before the main MDX loader, but based on the naming convention, a "pre-loader" should process the file before it's passed to the main loader.

If this is intentional and the pre-loader should run first, consider renaming it to "mdx-post-loader" to clarify the execution order. Otherwise, reverse the order:

{
  loader: require.resolve('@teambit/mdx.modules.mdx-pre-loader'),
},
{
  loader: require.resolve('@mdx-js/loader'),
  options: mdxOptions,
},

Copilot uses AI. Check for mistakes.
Comment on lines +162 to 167
loader: require.resolve('@mdx-js/loader'),
options: mdxOptions,
},
{
loader: require.resolve('@teambit/mdx.modules.mdx-pre-loader'),
},
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect loader order for MDX processing

Webpack loaders execute from bottom to top in the use array. The current order has:

  1. @mdx-js/loader (executed second)
  2. @teambit/mdx.modules.mdx-pre-loader (executed first)

This means the pre-loader runs before the main MDX loader, but based on the naming convention, a "pre-loader" should process the file before it's passed to the main loader.

If this is intentional and the pre-loader should run first, consider renaming it to "mdx-post-loader" to clarify the execution order. Otherwise, reverse the order:

{
  loader: require.resolve('@teambit/mdx.modules.mdx-pre-loader'),
},
{
  loader: require.resolve('@mdx-js/loader'),
  options: mdxOptions,
},
Suggested change
loader: require.resolve('@mdx-js/loader'),
options: mdxOptions,
},
{
loader: require.resolve('@teambit/mdx.modules.mdx-pre-loader'),
},
loader: require.resolve('@teambit/mdx.modules.mdx-pre-loader'),
},
{
loader: require.resolve('@mdx-js/loader'),
options: mdxOptions,
},

Copilot uses AI. Check for mistakes.
"@mdx-js/mdx": "1.6.21",
"@mdx-js/loader": "^3.1.1",
"@mdx-js/mdx": "^3.1.1",
"@mdx-js/react": "1.6.22",
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent MDX package versions

The @mdx-js/mdx and @mdx-js/loader packages have been updated to v3 (^3.1.1), but @mdx-js/react remains at v1 (1.6.22).

For MDX v3, @mdx-js/react should also be updated to v3 to ensure compatibility. The v1 version may not work correctly with MDX v3 compiled output.

Suggested update:

"@mdx-js/react": "^3.1.1"
Suggested change
"@mdx-js/react": "1.6.22",
"@mdx-js/react": "^3.1.1",

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings December 5, 2025 00:39
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 21 out of 22 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

import { humanizeCompositionId } from './humanize'

```jsx live=true
```jsx
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of live=true from the code block may affect functionality. The live=true attribute typically indicates that the code should be rendered as an interactive example. Verify that this functionality is still supported in MDX v3, or if an alternative approach is needed to maintain the live code preview feature.

Suggested change
```jsx
```jsx live=true

Copilot uses AI. Check for mistakes.
Comment on lines 208 to 209
{ logger: this.logger, worker } as any
// { logger: this.logger, worker, devFiles: this.devFiles },
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented-out code should be removed rather than left in the codebase. The commented line appears to be the correct implementation (including devFiles: this.devFiles), while the active line uses a type assertion (as any) to bypass type checking. Either the commented code should be uncommented and the type issue fixed properly, or it should be removed entirely if it's no longer needed.

Suggested change
{ logger: this.logger, worker } as any
// { logger: this.logger, worker, devFiles: this.devFiles },
{ logger: this.logger, worker, devFiles: this.devFiles }

Copilot uses AI. Check for mistakes.
Comment on lines 544 to 545
{ logger: this.logger, worker } as any
// { logger: this.logger, worker, devFiles: this.devFiles },
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented-out code should be removed rather than left in the codebase. The commented line appears to be the correct implementation (including devFiles: this.devFiles), while the active line uses a type assertion (as any) to bypass type checking. Either the commented code should be uncommented and the type issue fixed properly, or it should be removed entirely if it's no longer needed.

Suggested change
{ logger: this.logger, worker } as any
// { logger: this.logger, worker, devFiles: this.devFiles },
{ logger: this.logger, worker, devFiles: this.devFiles },

Copilot uses AI. Check for mistakes.
Comment on lines 278 to +283
"@teambit/legacy-bit-id": "^1.1.3",
"@teambit/mdx.compilers.mdx-transpiler": "^1.0.11",
"@teambit/mdx.generator.mdx-templates": "^1.0.14",
"@teambit/mdx.modules.mdx-loader": "^1.0.15",
"@teambit/mdx.modules.mdx-pre-loader": "^0.0.1",
"@teambit/mdx.modules.mdx-v3-options": "^0.0.2",
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of @teambit/mdx.compilers.mdx-transpiler from the dependency policy should be verified. Since this component is still imported and used in other files (though now replaced with @mdx-js/mdx's compileSync), ensure that all references to this package have been properly removed or updated throughout the codebase to avoid unused dependencies or broken imports.

Copilot uses AI. Check for mistakes.
To use this environment for your components, add it to any of the `variants` in your `workspace.jsonc` file as follows:

```json title="workspace.jsonc"
```json
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The removal of the title attribute from code blocks may reduce documentation clarity. The title="workspace.jsonc" attribute provided helpful context about which file the code example refers to. Consider whether this information should be retained through other means (e.g., comments or surrounding text) to maintain documentation quality.

Suggested change
```json
```json title="workspace.jsonc"

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings December 5, 2025 01:15
@Jinjiang Jinjiang force-pushed the jinjiang/mdx-update-20251202 branch from 8e204ba to 6c7edc9 Compare December 5, 2025 01:15
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

{ config: jestConfigPath, jest: jestPath },
{ logger: this.logger, worker, devFiles: this.devFiles }
{ logger: this.logger, worker } as any
// { logger: this.logger, worker, devFiles: this.devFiles },
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to line 209, there's commented-out code with uncertainty about the devFiles parameter. This should be resolved - either include the parameter or remove the commented code completely. Having commented code in production suggests incomplete migration.

Suggested change
// { logger: this.logger, worker, devFiles: this.devFiles },

Copilot uses AI. Check for mistakes.
}
}
``` -->
``` */}
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent comment syntax in the same file. Lines 24-25 use {/* */} (correct MDX v3 comment syntax for JSX context), while lines 172-229 use the same syntax. However, the closing comment on line 45 is standalone. Ensure all JSX-context comments consistently use the {/* */} syntax.

Suggested change
``` */}

*/}

Copilot uses AI. Check for mistakes.
Comment on lines +4 to 21

type ImportSpecifier = {
/**
* relative/absolute or module name. e.g. the `y` in the example of `import x from 'y';`
*/
fromModule: string;

/**
* is default import (e.g. `import x from 'y';`)
*/
isDefault?: boolean;

/**
* the name used to identify the module, e.g. the `x` in the example of `import x from 'y';`
*/
identifier?: string;
};

Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ImportSpecifier type is duplicated from what was likely exported by the old MDX transpiler. Consider importing this type from a shared location or the MDX package if available, rather than redefining it locally. This will reduce maintenance burden and potential drift from the canonical definition.

Suggested change
type ImportSpecifier = {
/**
* relative/absolute or module name. e.g. the `y` in the example of `import x from 'y';`
*/
fromModule: string;
/**
* is default import (e.g. `import x from 'y';`)
*/
isDefault?: boolean;
/**
* the name used to identify the module, e.g. the `x` in the example of `import x from 'y';`
*/
identifier?: string;
};
import type { ImportSpecifier } from '@mdx-js/mdx';

Copilot uses AI. Check for mistakes.
const output = await compile(contents.toString('utf-8'), { filepath: path });
const metadata = output.getMetadata();
const output = compileSync(contents.toString('utf-8'), mdxOptions);
const metadata = output.data.frontmatter as any;
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code accesses output.data.frontmatter without checking if output.data exists first. MDX v3's compileSync may not always populate the data property, which could lead to a runtime error. Consider adding a null check: const metadata = output.data?.frontmatter as any || {};

Suggested change
const metadata = output.data.frontmatter as any;
const metadata = output.data?.frontmatter as any || {};

Copilot uses AI. Check for mistakes.
const output = compileSync(source);
const imports = output.getImportSpecifiers();
const output = compileSync(source, mdxOptions);
const imports = output.data.imports as ImportSpecifier[];
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the doc-reader, accessing output.data.imports without checking if output.data exists could cause runtime errors. Add a null check: const imports = output.data?.imports as ImportSpecifier[] || []; to handle cases where the data property might be undefined.

Suggested change
const imports = output.data.imports as ImportSpecifier[];
const imports = output.data?.imports as ImportSpecifier[] || [];

Copilot uses AI. Check for mistakes.
Comment on lines +37 to +40
const afterMdxCompile = mdxCompileSync(fileContent, basicMdxOptions);
const filePathAfterMdxCompile = this.replaceFileExtToJs(options.filePath);
const afterBabelCompile = babelTranspileFileContent(
afterMdxCompile.contents,
afterMdxCompile.toString(),
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MDX v3 compileSync returns a VFile object, not a plain object with a contents property. You should call .toString() method on the result to get the compiled output. However, it appears this is already being done correctly on line 40. Please verify that basicMdxOptions is properly configured for the expected output format.

Copilot uses AI. Check for mistakes.
@Jinjiang Jinjiang force-pushed the jinjiang/mdx-update-20251202 branch from 6c7edc9 to 0506006 Compare December 5, 2025 01:36
Copilot AI review requested due to automatic review settings December 5, 2025 01:38
@Jinjiang Jinjiang force-pushed the jinjiang/mdx-update-20251202 branch from 0506006 to 731502c Compare December 5, 2025 01:38
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

const appType = new ReactAppType('react-app', reactEnv, logger, dependencyResolver);
const react = new ReactMain(reactEnv, envs, application, appType, dependencyResolver, logger);
graphql.register(() => reactSchema(react));
graphql.register(() => reactSchema(react) as any);
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GraphQL schema registration is cast to as any, suppressing type checking. This loses TypeScript's type safety benefits.

Investigate why this type assertion is needed and consider defining proper types for the schema instead of using as any.

Copilot uses AI. Check for mistakes.
const output = await compile(contents.toString('utf-8'), { filepath: path });
const metadata = output.getMetadata();
const output = compileSync(contents.toString('utf-8'), mdxOptions);
const metadata = output.data.frontmatter as any;
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MDX compilation result is cast to as any when accessing the frontmatter data. This loses type safety for the metadata extraction.

Consider defining a proper type interface for the MDX compilation output data structure instead of using as any.

Copilot uses AI. Check for mistakes.
@Jinjiang Jinjiang force-pushed the jinjiang/mdx-update-20251202 branch from 731502c to 59c7ef8 Compare December 5, 2025 03:19
Copilot AI review requested due to automatic review settings December 5, 2025 04:50
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

config,
},
{ logger: this.logger, worker }
// { logger: this.logger, worker } as any,
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commented-out code // { logger: this.logger, worker } as any, should be removed rather than left as a comment. Commented-out code clutters the codebase and can cause confusion about the intended implementation.

Suggested change
// { logger: this.logger, worker } as any,

Copilot uses AI. Check for mistakes.
const output = await compile(contents.toString('utf-8'), { filepath: path });
const metadata = output.getMetadata();
const output = compileSync(contents.toString('utf-8'), mdxOptions);
const metadata = output.data.frontmatter as any;
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In MDX v3, the compileSync function returns a VFile object, not a plain object with a data.frontmatter property. The frontmatter is typically extracted through remark plugins like remark-frontmatter and remark-mdx-frontmatter. Without proper plugin configuration in mdxOptions, accessing output.data.frontmatter may result in undefined at runtime, breaking the metadata extraction.

Suggested change
const metadata = output.data.frontmatter as any;
// Ensure mdxOptions includes remark-frontmatter and remark-mdx-frontmatter plugins for frontmatter extraction.
const metadata = (output.data && output.data.frontmatter) ? output.data.frontmatter : {};

Copilot uses AI. Check for mistakes.
Comment on lines +31 to 32
const imports = output.data.imports as ImportSpecifier[];
if (!imports) return [];
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In MDX v3, the compileSync function returns a VFile object which needs to be converted to a string using .toString() or accessed via .value property. The data.imports property may not exist unless specific plugins are configured to populate it. The expected structure for imports in MDX v3 is different from v1, and without proper plugin configuration, this will likely fail at runtime.

Suggested change
const imports = output.data.imports as ImportSpecifier[];
if (!imports) return [];
const imports = Array.isArray(output.data?.imports) ? output.data.imports as ImportSpecifier[] : [];
if (!imports.length) return [];

Copilot uses AI. Check for mistakes.
// Make sure the mdx-loader is a dependency
import '@teambit/mdx.modules.mdx-loader';

// eslint-disable-next-line import/no-unresolved
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The /* eslint-disable import/no-unresolved */ comment suggests there's an unresolved import issue with @mdx-js/loader. This typically indicates that the package is not properly installed or the types are not available. This should be resolved rather than suppressed, as it may cause issues in development and could indicate a missing dependency.

Suggested change
// eslint-disable-next-line import/no-unresolved

Copilot uses AI. Check for mistakes.
}
}
`;
` as any;
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple as any type assertions are being added to GraphQL query definitions. While this might resolve immediate TypeScript compilation issues related to the MDX v3 upgrade, it bypasses type safety and could hide real type errors. Consider using proper typing with GraphQL code generation tools like graphql-code-generator or explicitly typing the queries instead of using as any.

Copilot uses AI. Check for mistakes.

```jsx live=true
```jsx live="true"
<LoginForm><LoginForm/>
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The closing tag syntax is incorrect. It should be <LoginForm /> with a space before the closing slash, not <LoginForm/> (which is also missing the opening tag based on line 24). This appears to be a typo that would cause a syntax error.

Suggested change
<LoginForm><LoginForm/>
<LoginForm />

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants