feat(galacean): add conditional exports#3058
Conversation
WalkthroughThe top-level ChangesPackage Exports Configuration
Estimated code review effort: 1 (Trivial) | ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3058 +/- ##
===========================================
- Coverage 79.37% 79.36% -0.02%
===========================================
Files 903 903
Lines 100632 100632
Branches 11260 11256 -4
===========================================
- Hits 79879 79865 -14
- Misses 20569 20583 +14
Partials 184 184
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/galacean/package.json (1)
17-24: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMissing
"./package.json"subpath export.Once
"exports"is defined, only listed subpaths remain accessible — tools that read the dependency's ownpackage.json(bundlers, React Native/Metro,require.resolve('galacean/package.json'), etc.) will fail withERR_PACKAGE_PATH_NOT_EXPORTED. This is a well-known gotcha (see nodejs/node#33460, react-native-community/cli#1168) that has broken several popular packages after adoptingexports.🔧 Suggested addition
"exports": { ".": { "types": "./types/index.d.ts", "import": "./dist/module.js", "browser": "./dist/browser.js", "default": "./dist/main.js" - } + }, + "./package.json": "./package.json" },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/galacean/package.json` around lines 17 - 24, The package exports map in the package.json currently omits the package’s own package.json subpath, which breaks consumers that resolve it directly. Update the exports block in the package manifest to include the "./package.json" subpath alongside the existing "." entry so tools like bundlers, Metro, and require.resolve can access it. Keep the fix localized to the package.json exports definition and preserve the current main/browser/module/type entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/galacean/package.json`:
- Around line 17-24: The package exports map in the galacean package is ordered
incorrectly, causing resolvers that match both conditions to select the import
target before the browser build. Update the object in the exports entry so the
browser condition is listed before import, keeping the types entry unchanged and
preserving the existing default fallback.
---
Nitpick comments:
In `@packages/galacean/package.json`:
- Around line 17-24: The package exports map in the package.json currently omits
the package’s own package.json subpath, which breaks consumers that resolve it
directly. Update the exports block in the package manifest to include the
"./package.json" subpath alongside the existing "." entry so tools like
bundlers, Metro, and require.resolve can access it. Keep the fix localized to
the package.json exports definition and preserve the current
main/browser/module/type entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f2285e36-c5e1-4739-9a7c-dfcab3783759
📒 Files selected for processing (3)
packages/galacean/package.jsonpackages/galacean/src/ShaderPool.tspackages/galacean/src/index.ts
| "exports": { | ||
| ".": { | ||
| "types": "./types/index.d.ts", | ||
| "import": "./dist/module.js", | ||
| "browser": "./dist/browser.js", | ||
| "default": "./dist/main.js" | ||
| } | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What condition key order do Vite, Webpack 5, and Rollup use when resolving package.json "exports" for browser + ESM builds, and does key order in the exports object take priority over the resolver's condition list order?
💡 Result:
Vite, Webpack 5, and Rollup all adhere to the Node.js module resolution specification, which mandates that within the "exports" object of a package.json file, key order is significant [1][2][3]. During conditional export resolution, keys are checked in the order they appear in the object; the first matching condition found is the one that takes precedence [2][4][5]. Because the resolution process stops as soon as a match is found, the order of keys in the "exports" object effectively defines the priority of your conditions [2][4]. The standard practice is to list conditions from most specific to least specific (e.g., placing custom environment conditions before general fallbacks like "default") [5][3]. Regarding your question on priority: key order within the "exports" object takes absolute priority over the order in which you might list or "enable" conditions in a resolver's configuration [1][2]. While the bundler's configuration (such as Webpack's resolve.conditionNames or Vite/Rollup's export conditions) determines which conditions are currently "active" or available to be matched, the resolver will only select a condition if it exists in that active list AND appears earliest in the "exports" object's defined order [1][2][4]. Consequently, if you have both "import" and "require" active in your configuration, the one that appears first in your package.json "exports" object will be the one chosen [4].
Citations:
- 1: Module Resolution: Aligning
package.jsonexports, conditional exports, and resolution algorithms across bundlers openjs-foundation/bundler-collab-space#7 - 2: Exports and
conditionNamesmatching webpack/enhanced-resolve#318 - 3: https://github.com/nodejs/node/blob/HEAD/doc/api/packages.md
- 4: Do not add default
node,browser,requireorimportconditions lukeed/resolve.exports#12 - 5: https://nodejs.org/docs/latest-v23.x/api/packages.html
Place browser before import in the package exports map
exports resolution follows object key order, so with "import" first, browser ESM resolvers that enable both conditions will match ./dist/module.js before ./dist/browser.js. Move "browser" ahead of "import" to make the browser build selectable.
Suggested fix
"exports": {
".": {
"types": "./types/index.d.ts",
- "import": "./dist/module.js",
"browser": "./dist/browser.js",
+ "import": "./dist/module.js",
"default": "./dist/main.js"
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "exports": { | |
| ".": { | |
| "types": "./types/index.d.ts", | |
| "import": "./dist/module.js", | |
| "browser": "./dist/browser.js", | |
| "default": "./dist/main.js" | |
| } | |
| }, | |
| "exports": { | |
| ".": { | |
| "types": "./types/index.d.ts", | |
| "browser": "./dist/browser.js", | |
| "import": "./dist/module.js", | |
| "default": "./dist/main.js" | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/galacean/package.json` around lines 17 - 24, The package exports map
in the galacean package is ordered incorrectly, causing resolvers that match
both conditions to select the import target before the browser build. Update the
object in the exports entry so the browser condition is listed before import,
keeping the types entry unchanged and preserving the existing default fallback.
There was a problem hiding this comment.
@luo2430 Got it, kicking off the change to reorder the exports map.
- Reorder
exports["."]inpackages/galacean/package.jsonsobrowserprecedesimport, keepingtypesfirst anddefaultlast.
Failed to handle agent chat message. Please try again.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
两个 commit：feat 给 @galacean/engine 加 exports 条件导出映射；refactor 把 3 处 @ts-ignore→@ts-expect-error 并把注册循环从 for...in 改 Object.entries。refactor 部分全部正确，但 exports 映射的条件排序有方向性错误——而且 CodeRabbit 建议、作者已回复 "apply" 的那个"修复"会把问题从「无效」变成「有害」。请勿 apply 那个 diff。
先厘清三个产物的真实格式（来自 rollup.config.js）：
| dist 文件 | 格式 | 用途 |
|---|---|---|
dist/module.js |
ES module（format:"es"，可 tree-shake） |
ESM import |
dist/browser.js |
UMD（format:"umd"，全局 Galacean） |
<script> 标签 |
dist/main.js |
CommonJS（format:"commonjs"） |
require / node |
关键：browser.js 是 UMD，不是「浏览器版 ESM」。
问题
[P1] packages/galacean/package.json:17-24 — exports 里把 UMD 挂到 browser 条件，排序两难，正确解是删掉这个条件
exports 条件按对象键顺序匹配，且 browser 是格式无关的环境条件（只表示"浏览器变体"，不表示 ESM/CJS）。而这里 browser 指向的是 UMD bundle。于是：
-
当前状态（
import在前，browser在后）：所有现代打包器（Vite/Webpack5/Rollup）在浏览器 ESM 模式下同时启用browser和import，键顺序让import→module.js(ESM) 先命中。browser这条永远轮不到，是死条件——只有"启用 browser 但不启用 import"的解析器才会命中它，现实中不存在。所以现在不会坏，但browser条目毫无作用。 -
CodeRabbit 建议 + 作者已回 "apply" 的改法（
browser提到import前）：浏览器打包器会先命中browser→拿到 UMDbrowser.js，而不是可 tree-shake 的 ESMmodule.js。这是实打实的退化：UMD 不可 tree-shake、破坏静态import分析、bundle 体积变大。方向反了，请不要 apply。
根因是「把 UMD 产物放进 exports 的 browser 条件」这件事本身就错——browser 条件的语义是"给浏览器用的模块变体"，正确填法应是浏览器用的 ESM（也就是 module.js），而不是 UMD。但 module.js 已经由 import 覆盖，所以 browser 条件在 exports 里没有独立价值。
建议：exports 里直接删掉 browser 条件，只保留标准三段：
"exports": {
".": {
"types": "./types/index.d.ts",
"import": "./dist/module.js",
"require": "./dist/main.js",
"default": "./dist/main.js"
}
}（把兜底的 default 语义显式拆出 require+default，比单个 default 更清晰；也可只留 import+default。）UMD 的 browser.js 继续通过 CDN 的 unpkg/jsdelivr 字段和 <script> 直链分发，不需要进 exports。
竞品对标：Three.js（Web 引擎标杆）的 exports 只有 import/require 两条，根本不放 browser 条件——它的 three.module.js(ESM) 就是浏览器构建，因为现代打包器要的就是 ESM。UMD 只走 <script>，不进 exports。Galacean 这里应对齐同一模型。
补充一个副作用提示（非阻塞）：新增 exports 后，任何支持 exports 的解析器会以 exports 为唯一入口，顶层 main/module 字段被其覆盖。本包只导出 barrel、无深层子路径导入，所以安全；但这属于 module resolution 的语义变更，建议在 commit body 里点一句 BREAKING/behavior-change 备案。
已验证无问题的改动（回应作者"仍需验证"）
index.ts注册循环for...in→Object.entries：等价且正确。import * as CoreObjects是 module namespace object——原型为null、所有导出都是 own+enumerable 的字符串键。for...in（无原型链可走）与Object.entries在此枚举完全相同的键集，不会漏注册任何类；export type只存在于类型层、运行时已被擦除不进 namespace。改动同时消除了CoreObjects[key]的 TS 索引类型报错。作者担心的"循环原型链"其实对 namespace object 不成立（原型是 null），但改法本身仍是净改进。无需再验证。- 3 处
@ts-ignore→@ts-expect-error：安全且更优。挂在_createFromPrecompiled/_feedbackVaryings/SystemInfo._initialize三处真实@internal违规上，确有 TS error，@ts-expect-error合法；一旦这些成员将来转公开，@ts-expect-error会主动报"未使用的抑制"提醒清理，比@ts-ignore强。index.ts删掉version行上方的//@ts-ignore也安全——\__buildVersion`是普通模板字面量、编译期无 error；若真有 error，b:types`(tsc) 会拦。
问题（元信息）
[P2] PR 标题混装 feat + refactor
标题 feat(galacean): ... | refactor(galacean): ... 把一个新功能和一次重构塞进同一 PR。这会影响 changelog 生成和回滚粒度（revert 一个会连带丢掉另一个）。建议拆成两个 PR，或标题只体现主体变更类型。提一次，不阻塞。
自检：refactor 两项已逐一从第一性原理验证等价性（namespace object 枚举语义 / @ts-expect-error 触发条件）；exports 结论与 CodeRabbit 相反（其建议方向错误），非重复而是纠正；产物格式均已从 rollup.config.js 实证（browser.js=UMD）。
luo2430
changed the title
|
改好了，不会是还没睡吧 |
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
作者把前一版整体 revert（ffa133f02 撤 refactor、7bae7d55f 撤旧 feat），重新以单文件 e4cfc2d9e feat(galacean): add conditional exports 落地——packages/galacean/package.json 净加 7 行 exports 块，其余全部退出本 PR。方向完全正确，上轮唯一阻塞 P1 已闭环。
上轮问题闭环情况
- [P1]
exports里 UMD 挂browser条件 → ✅ 已按我的建议修复。新exports只保留标准三段types/import/default，没有browser条件。作者虽然对 CodeRabbit 的 inline 回了 "apply"，但并未真的 apply 那个「browser 提到 import 前」的有害改法（那会让浏览器打包器拿到不可 tree-shake 的 UMD），而是走了删掉browser条件的正解。对齐 Three.js 模型（exports只放 import/require 类条件，UMD 走顶层字段 + CDN）。 - [P2] 标题混装 feat+refactor → ✅ 已消除。refactor commit 已 revert，PR 现在是纯
feat、单文件。
本轮复核（新 HEAD e4cfc2d9e）
逐条从第一性原理确认最终形态正确：
types在首位 ✅（Node 规范要求类型条件先于运行时条件命中）import→./dist/module.js（ESM，可 tree-shake）✅default→./dist/main.js（CJS，兜底覆盖require及其他解析器）✅- 顶层
browser: "dist/browser.js"字段保留且留在exports之外——这是对的：legacy browserify/webpack4 的 browser-field remap 独立于exports，保留无害；unpkg/jsdelivr是 CDN 专用非标准字段，不受exports影响。 - 顺带确认了那条上轮标注的非阻塞 side-effect（
exports一旦存在即成为"."的唯一解析入口、覆盖顶层字段）确实安全：全仓 grep 无任何@galacean/engine/<subpath>深层导入，消费者一律走 barrel，exports只导出"."不 break 任何内部依赖。
无 P0/P1/P2/P3 新问题。之前的 CHANGES_REQUESTED 门控随本次 approve 解除。LGTM。
2 participants
主要用于适配在一个utils包中包装部分功能并且
export * from "@galacean/engine";以方便其它库使用的情况。