-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
fix: add react-helmet-async for alias #12511
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Walkthrough这些更改在 Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/bundler-vite/src/config/transformer/alias.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/bundler-vite/src/config/transformer/alias.ts
@@ -39,6 +39,7 @@ export default (function alias(userConfig) { | |||
alias: [ | |||
// to support less-loader ~ for local deps, refer: https://github.com/vitejs/vite/issues/2185 | |||
{ find: /^~/, replacement: '' }, | |||
{ find: /react-helmet-async/, replacement: 'react-helmet-async' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里不需要精确限定正则开头和结尾吗?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个包未来依赖的版本会变化吗?如果不会变化或者确定暂时不会变化的话,可以锁定开头和结尾的
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
主要是担心如果锁定开头和结尾,如果未来依赖的新的包,而这里没有进行修改的话,会不会再次产生这个问题?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
逻辑放到 packages/preset-umi/src/features/vite/vite.ts 下吧、加一下 window 判断 import { isWindows } from '../../utils/platform' 在增加别名逻辑 理论上直接控制 modifyConfig 就行 可以试试下面的代码
// packages/preset-umi/src/features/vite/vite.ts
import { isWindows } from '../../utils/platform'
api.modifyConfig((memo) => {
if (isWindows) {
memo.alias['react-helmet-async'] = 'react-helmet-async'
}
return memo;
});
@xierenyuan review 下? |
另外,ci 挂了。 |
需要修理一下单测,参考 ci 结果。 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #12511 +/- ##
==========================================
- Coverage 28.04% 28.03% -0.02%
==========================================
Files 497 498 +1
Lines 15698 15704 +6
Branches 3764 3766 +2
==========================================
Hits 4402 4402
- Misses 10498 10504 +6
Partials 798 798 ☔ View full report in Codecov by Sentry. |
@SK-Luffa 辛苦看下我的评论修改后测试下(我没有 window 电脑) 辛苦了 |
@@ -19,6 +20,9 @@ export default (api: IApi) => { | |||
|
|||
api.modifyConfig((memo) => { | |||
// like vite, use to pre-bundling dependencies in vite mode | |||
if (isWindows) { | |||
memo.alias['react-helmet-async'] = require.resolve('react-helmet-async'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个依赖在 renderer-react
下,不在 preset-umi
包里面,在 pnpm 严格的依赖结构下是获取不到的吧(此问题无法在此仓库内测试出来,但一般认为 require.resolve
在严格模式下只能取自己包的依赖)。
如果非要找这个包的绝对位置,应该先找 renderer-react
,之后基于 renderer-react
的位置作为起点找 react-helmet-async
(参考 plugins/src/react-query
里找 @tanstack/query-core
的位置的代码)。
为了避免找绝对路径比较麻烦,值直接写包名可以吗?
const rendererReactPath = resolveProjectDep({ | ||
pkg: api.pkg, | ||
cwd: api.cwd, | ||
dep: RENDERER_REACT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里要先从本级包 preset-umi
开始找他依赖的 renderer-react
,起点是本身(因为 preset-umi
直接依赖了 renderer-react
),可以用 require.resolve
,不指定 paths
时默认从当前文件开始找,往上找也就是本级包开始找。
而 resolveProjectDep
是从用户的项目作为起点开始找,用户并不直接依赖 renderer-react
,所以严格的依赖结构里找不到,这里和 RQ 插件那边还是有一些区别的,因为那边 resolveProjectDep
的本意是,如果用户自己安装了,就使用 用户自己安装的,否则用自己依赖的(从本级找),而这里只是内部寻找,和用户依赖无关,但原理是一样的。
import { dirname } from 'path'; | ||
import { resolveProjectDep } from '../../utils/resolveProjectDep'; | ||
|
||
// 解析react-helmet-async 在renderer-react中的路径 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
另外这里还有一些细节问题,比如没有盘古之白,上面的导入中间有一个空行。
#10212 在windows上 报错的原因是因为这个库的esm版本内的依赖 仍是comjs 但是vite没有识别到,所以需要在vite内resolve 的alias 添加上react-helmet-async 我测试过在umi脚手架生成的项目内添加是完全可以解决的
但是不知道在源码里这样改是否正确
Summary by CodeRabbit
bundler-vite
包中新增了别名规则,将react-helmet-async
替换为自身。