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.

Sign up for GitHub

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

Jump to bottom

feat: support npm alias name for prebundle #663

Merged
merged 5 commits into from
Jun 14, 2023
Merged

feat: support npm alias name for prebundle #663

merged 5 commits into from
Jun 14, 2023

Conversation

Dunqing
Copy link
Contributor

@Dunqing Dunqing commented Jun 12, 2023 •
edited
Loading

close: #662

@codecov
Copy link

codecov bot commented Jun 12, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (10a4994) 93.77% compared to head (c246793) 93.79%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #663      +/-   ##
==========================================
+ Coverage   93.77%   93.79%   +0.02%     
==========================================
  Files          55       54       -1     
  Lines        1525     1531       +6     
  Branches      361      364       +3     
==========================================
+ Hits         1430     1436       +6     
  Misses         95       95
Impacted Files Coverage Δ
src/doctor/rules/CJS_IMPORT_ESM.ts 33.33% <100.00%> (-2.39%) ⬇️
src/doctor/rules/PHANTOM_DEPS.ts 100.00% <100.00%> (ø)
src/prebundler/config.ts 100.00% <100.00%> (ø)
src/utils.ts 98.68% <100.00%> (+0.15%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

PeachScript
PeachScript reviewed Jun 13, 2023
View reviewed changes
src/prebundler/config.ts
@@ -147,7 +147,7 @@ export function getConfig(opts: {
pkg: depPkg,
output: path.resolve(
opts.cwd,
`${output || DEFAULT_OUTPUT_DIR}/${depPkg.name}/index.js`,
`${output || DEFAULT_OUTPUT_DIR}/${depName}/index.js`,
Copy link
Member

Choose a reason for hiding this comment

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

直接使用用户的配置值可能会存在 Breaking Change，目前想到这两种 case：

  1. 配置值可能是 NPM 包的文件路径（类似 pkg/lib/other）
  2. 配置值可能是项目的本地文件（类似 Umi 仓库中 bundler-utils 子包的情况，虽然没有直接使用 father）

想到的解法你看看是否合适，创建 getDepPkgName(depName, depPkg) 的工具方法，有两层逻辑：

  1. 如果 depName 不是绝对路径或 . 开头的相对路径，则尝试截取 depName 中包名的部分，比如 pkg/lib/other => pkg，@org/pkg/index => @org/pkg
  2. 反之兜底到 depPkg.name，同时需要看下 issue 里提到的 minimatch 获取 pkg 失败的问题，按理说 getDepPkg 有使用 pkgUp 处理，应该能拿到才是

Copy link
Contributor Author

Choose a reason for hiding this comment

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

很好我处理下，我没留意还有其他用法

@Dunqing
Copy link
Contributor Author

Dunqing commented Jun 13, 2023

关于 minimatch 的问题另外处理，这里只做 npm alias 的支持。你觉得呢？ @PeachScript

PeachScript
PeachScript reviewed Jun 13, 2023
View reviewed changes
src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
@PeachScript PeachScript changed the title feat(prebundle): support npm alias name feat: support npm alias name for prebundle Jun 14, 2023
PeachScript
PeachScript approved these changes Jun 14, 2023
View reviewed changes
Copy link
Member

@PeachScript PeachScript left a comment

Choose a reason for hiding this comment

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

+1

@PeachScript PeachScript merged commit 8df2e42 into umijs:master Jun 14, 2023
@PeachScript PeachScript mentioned this pull request Jun 15, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PeachScript PeachScript PeachScript approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

预编译的输出目录， 是否应该 name 为目录名称， 而不是 depPkg.name
2 participants
@Dunqing @PeachScript