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

Change webpack peer dependency to optional in loader #2440

Merged
merged 2 commits into from
Feb 13, 2024

Conversation

chenjiahan
Copy link
Contributor

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Rspack is a fast Rust-based web bundler and it is compatible with the architecture and ecosystem of webpack.

The @mdx-js/loader can be used with Rspack, but users will get a peer dependency warning, adding peerDependenciesMeta can resolve it.

The same as: jantimon/html-webpack-plugin#1829

Copy link

vercel bot commented Feb 13, 2024

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

Name Status Preview Comments Updated (UTC)
mdx ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 13, 2024 11:44am

Copy link
Member

@remcohaszing remcohaszing left a comment

Choose a reason for hiding this comment

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

I think all modern package managers install peer dependencies. They don’t install optional peer dependencies. The ones that don’t install peer dependencies merely show a warning, which is annoying, but not an error.

The webpack peer dependency exists for 2 reasons:

  1. To indicate this is only compatible with Webpack versions 5 or greater.
  2. For the type definitions, which users may or may not use. This is really similar to the @types/react peer dependency, which was discussed in react: fix to classify @types/react as a peer dependency #2281. We ended up not using optional peer dependencies there.

packages/loader/package.json Outdated Show resolved Hide resolved
packages/loader/package.json Outdated Show resolved Hide resolved
packages/loader/readme.md Outdated Show resolved Hide resolved
packages/loader/readme.md Outdated Show resolved Hide resolved
@wooorm
Copy link
Member

wooorm commented Feb 13, 2024

typescript users that use rspack should likely still have webpack for types to work?
How likely is it that rspack users also use typescript?

If rspack supports webpack loaders, we don’t have to change the docs from webpack loader to webpack or rspack loader. Rspack users know that they can use webpack loaders.

Co-authored-by: Remco Haszing <[email protected]>
Signed-off-by: neverland <[email protected]>
@chenjiahan chenjiahan changed the title Add Rspack as an optional peer dependency of loader Mark webpack as an optional peer dependency of loader Feb 13, 2024
@chenjiahan
Copy link
Contributor Author

Thank you for all the review comments, I have updated the PR content, just treating webpack as an optional peer dependency.
Some Rspack users will use TypeScript, but the types of webpack are not necessary in most scenarios, so it might be more appropriate to not install webpack.

@wooorm
Copy link
Member

wooorm commented Feb 13, 2024

Some Rspack users will use TypeScript, but the types of webpack are not necessary in most scenarios, so it might be more appropriate to not install webpack.

How do you configure loaders such as this in rspack then?
It is IMO quite useful to have type safety for the options of this loader. Those types are in this package. And they need webpack

@chenjiahan
Copy link
Contributor Author

The loader configuration in Rspack is completely the same as webpack.

As far as I know, at present, both webpack and Rspack do not provide type inference for loaders because loaders are registered in the webpack configuration through the loader's name or path, not by importing a loader package.

In my use case, I manually imported the type exported by @mdx-js/loader because I am developing a build tool based on Rspack.

https://github.com/web-infra-dev/rsbuild/blob/main/packages/plugin-mdx/src/index.ts#L2-L14

@wooorm
Copy link
Member

wooorm commented Feb 13, 2024

You can use TS outside of .ts files! With @type jsdoc. That’s what is done in the Use example here: https://github.com/mdx-js/mdx/tree/main/packages/loader#use. We recommend it to prevent mistakes.

@chenjiahan
Copy link
Contributor Author

This is a great method 👍🏻

As far as I know, import('@mdx-js/loader').Options does not depend on the types of webpack, webpack only provides the LoaderContext type, which will not affect Options type.

@remcohaszing
Copy link
Member

As far as I know, import('@mdx-js/loader').Options does not depend on the types of webpack, webpack only provides the LoaderContext type, which will not affect Options type.

This is true. However, the package also exports the type of the loader, which does depend on the webpack types.

I’m indifferent on whether or not the webpack peer dependency should be optional.

I do welcome the documentation change.

@wooorm
Copy link
Member

wooorm commented Feb 13, 2024

I’m fine with trying this out. But I am very worried about things breaking.

@wooorm wooorm changed the title Mark webpack as an optional peer dependency of loader Change webpack peer dependency to optional in loader Feb 13, 2024
@wooorm wooorm merged commit be79212 into mdx-js:main Feb 13, 2024
6 checks passed
@wooorm
Copy link
Member

wooorm commented Feb 13, 2024

Thank you :)

@chenjiahan chenjiahan deleted the loader_rspack_0213 branch February 14, 2024 02:20
@chenjiahan
Copy link
Contributor Author

Thank you for your time ❤️

@codecov-commenter

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants