feat(plugin-react): use @lynx-js/type-config for lynx config#1983
feat(plugin-react): use @lynx-js/type-config for lynx config#1983upupming wants to merge 19 commits intolynx-family:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 57ae232 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughRestructures public API types for React plugin Lynx configuration by introducing layered type composition (ReactLynxDefaultCompilerOptions, ReactLynxDefaultLynxConfig, ReactLynxOptions) and adding new packages ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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. 📢 Thoughts on this report? Let us know! |
upupming
force-pushed
the
feat/adopt-type-config
branch
from
c9924c6 to
2cbab2b
Compare
upupming
force-pushed
the
feat/adopt-type-config
branch
from
2cbab2b to
c96605f
Compare
CodSpeed Performance ReportMerging #1983 will degrade performances by 5.53%Comparing Summary
Benchmarks breakdown
Footnotes
|
React Example#6392 Bundle Size — 236.9KiB (0%).57ae232(current) vs 6400f87 main#6360(baseline) Bundle metrics
|
| Current #6392 |
Baseline #6360 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
162 |
162 |
| Duplicate Modules |
65 |
65 |
| Duplicate Code |
46.75% |
46.75% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #6392 |
Baseline #6360 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.14KiB |
91.14KiB |
Bundle analysis report Branch upupming:feat/adopt-type-config Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#6552 Bundle Size — 372.73KiB (0%).57ae232(current) vs 6400f87 main#6520(baseline) Bundle metrics
|
| Current #6552 |
Baseline #6520 |
|
|---|---|---|
| Initial JS |
146.31KiB |
146.31KiB |
| Initial CSS |
32.4KiB |
32.4KiB |
 Cache Invalidation |
0% |
11.74% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
| Modules |
230 |
230 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
2.97% |
2.97% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #6552 |
Baseline #6520 |
|
|---|---|---|
| JS |
243.35KiB |
243.35KiB |
| Other |
96.98KiB |
96.98KiB |
| CSS |
32.4KiB |
32.4KiB |
Bundle analysis report Branch upupming:feat/adopt-type-config Project dashboard
Generated by RelativeCI Documentation Report issue
upupming
force-pushed
the
feat/adopt-type-config
branch
from
cd43382 to
3aa8186
Compare
upupming
force-pushed
the
feat/adopt-type-config
branch
from
3aa8186 to
18d3c02
Compare
|
|
|
|
||
| ```json | ||
| {"content":"eyJjb21waWxlck9wdGlvbnMiOnsiZW5hYmxlRmliZXJBcmNoIjp0cnVlLCJ1c2VMZXB1c05HIjp0cnVlLCJlbmFibGVSZXVzZUNvbnRleHQiOnRydWUsImJ1bmRsZU1vZHVsZU1vZGUiOiJSZXR1cm5CeUZ1bmN0aW9uIiwiZGVidWdJbmZvT3V0c2lkZSI6dHJ1ZSwiZGVmYXVsdERpc3BsYXlMaW5lYXIiOnRydWUsImVuYWJsZUNTU0ludmFsaWRhdGlvbiI6ZmFsc2UsImVuYWJsZUNTU1NlbGVjdG9yIjp0cnVlLCJlbmFibGVMZXB1c0RlYnVnIjp0cnVlLCJlbmFibGVSZW1vdmVDU1NTY29wZSI6ZmFsc2UsInRhcmdldFNka1ZlcnNpb24iOiIzLjIiLCJkZWZhdWx0T3ZlcmZsb3dWaXNpYmxlIjp0cnVlfSwic291cmNlQ29udGVudCI6eyJkc2wiOiJyZWFjdF9ub2RpZmYiLCJhcHBUeXBlIjoiY2FyZCIsImNvbmZpZyI6eyJsZXB1c1N0cmljdCI6dHJ1ZSwidXNlTmV3U3dpcGVyIjp0cnVlLCJlbmFibGVOZXdJbnRlcnNlY3Rpb25PYnNlcnZlciI6dHJ1ZSwiZW5hYmxlTmF0aXZlTGlzdCI6dHJ1ZSwiZW5hYmxlQTExeSI6dHJ1ZSwiZW5hYmxlQWNjZXNzaWJpbGl0eUVsZW1lbnQiOmZhbHNlLCJlbmFibGVDU1NJbmhlcml0YW5jZSI6ZmFsc2UsImVuYWJsZU5ld0dlc3R1cmUiOmZhbHNlLCJyZW1vdmVEZXNjZW5kYW50U2VsZWN0b3JTY29wZSI6ZmFsc2V9fSwiY3NzIjp7ImNzc01hcCI6eyIwIjpbeyJ0eXBlIjoiU3R5bGVSdWxlIiwic3R5bGUiOlt7Im5hbWUiOiJjb2xvciIsInZhbHVlIjoiXCJibHVlXCIiLCJrZXlMb2MiOnsibGluZSI6MSwiYXsdW1uIjoxMXXsInZhbExvYyI6eyJsaW5lIjoxLCJjb2x1bW4iOjE5fX1dLCJzZWxlY3RvclRleHQiOnsidmFsdWUiOiIuZm9vIiwibG9jIjp7ImxpbmUiOjEsImNvbHVtbiI6NX19LCJ2YXJpYWJsZXMiOnt9fV19LCJjc3NTb3VyY2UiOnsiMCI6Ii9jc3NJZC8wLmNzcyJ9LCJjb250ZW50TWFwIjp7fXXsImxlcHVzQ29kZSI6eyJsZXB1c0NodW5rIjp7fXXsIm1hbmlmZXN0Ijp7fSwiY3VzdG9tU2VjdGlvbnMiOnt9fQ==","deps":{"0":[]}} | ||
| {"content":"eyJjb21waWxlck9wdGlvbnMiOnsiZW5hYmxlRmliZXJBcmNoIjp0cnVlLCJ1c2VMZXB1c05HIjp0cnVlLCJlbmFibGVSZXVzZUNvbnRleHQiOnRydWUsImJ1bmRsZU1vZHVsZU1vZGUiOiJSZXR1cm5CeUZ1bmN0aW9uIiwiZW5hYmxlTGVwdXNEZWJ1ZyI6dHJ1ZSwiZGVidWdJbmZvT3V0c2lkZSI6dHJ1ZSwiZGVmYXVsdERpc3BsYXlMaW5lYXIiOnRydWUsImRlZmF1bHRPdmVyZmxvd1Zpc2libGUiOnRydWUsImVuYWJsZUNTU0ludmFsaWRhdGlvbiI6ZmFsc2UsImVuYWJsZUNTU1NlbGVjdG9yIjp0cnVlLCJlbmFibGVSZW1vdmVDU1NTY29wZSI6ZmFsc2UsInRhcmdldFNka1ZlcnNpb24iOiIzLjIifSwic291cmNlQ29udGVudCI6eyJkc2wiOiJyZWFjdF9ub2RpZmYiLCJhcHBUeXBlIjoiY2FyZCIsImNvbmZpZyI6eyJsZXB1c1N0cmljdCI6dHJ1ZSwidXNlTmV3U3dpcGVyIjp0cnVlLCJlbmFibGVOZXdJbnRlcnNlY3Rpb25PYnNlcnZlciI6dHJ1ZSwiZW5hYmxlTmF0aXZlTGlzdCI6dHJ1ZSwiZW5hYmxlQTExeSI6dHJ1ZSwiZW5hYmxlQWNjZXNzaWJpbGl0eUVsZW1lbnQiOmZhbHNlLCJlbmFibGVDU1NJbmhlcml0YW5jZSI6ZmFsc2UsImVuYWJsZU5ld0dlc3R1cmUiOmZhbHNlLCJyZW1vdmVEZXNjZW5kYW50U2VsZWN0b3JTY29wZSI6ZmFsc2V9fSwiY3NzIjp7ImNzc01hcCI6eyIwIjpbeyJ0eXBlIjoiU3R5bGVSdWxlIiwic3R5bGUiOlt7Im5hbWUiOiJjb2xvciIsInZhbHVlIjoiXCJibHVlXCIiLCJrZXlMb2MiOnsibGluZSI6MSwiYXsdW1uIjoxMXXsInZhbExvYyI6eyJsaW5lIjoxLCJjb2x1bW4iOjE5fX1dLCJzZWxlY3RvclRleHQiOnsidmFsdWUiOiIuZm9vIiwibG9jIjp7ImxpbmUiOjEsImNvbHVtbiI6NX19LCJ2YXJpYWJsZXMiOnt9fV19LCJjc3NTb3VyY2UiOnsiMCI6Ii9jc3NJZC8wLmNzcyJ9LCJjb250ZW50TWFwIjp7fXXsImxlcHVzQ29kZSI6eyJsZXB1c0NodW5rIjp7fXXsIm1hbmlmZXN0Ijp7fSwiY3VzdG9tU2VjdGlvbnMiOnt9fQ==","deps":{"0":[]}} |
There was a problem hiding this comment.
This is updated just because config order is changed, no value is changed/added/deleted.
upupming
force-pushed
the
feat/adopt-type-config
branch
from
a69fed2 to
816ad5b
Compare
| "@lynx-js/react-webpack-plugin": "workspace:*", | ||
| "@lynx-js/runtime-wrapper-webpack-plugin": "workspace:*", | ||
| "@lynx-js/template-webpack-plugin": "workspace:*", | ||
| "@lynx-js/type-config": "npm:@upupming/type-config@0.0.1", |
There was a problem hiding this comment.
TODO: update this to official release
upupming
force-pushed
the
feat/adopt-type-config
branch
from
44dfbf3 to
37eee82
Compare
upupming
force-pushed
the
feat/adopt-type-config
branch
from
8366de2 to
ff5b697
Compare
![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
♻️ Duplicate comments (2)
packages/webpack/css-extract-webpack-plugin/test/hotCases/css/basic/__snapshot__/rspack/1.snap.txt (1)
17-17: Snapshot content update is consistent with regenerated outputOnly the JSON
contentbase64 payload changed; structure and deps are untouched. For this auto-generated snapshot, updating it to reflect new output (e.g., config order changes) is correct and needs no further action. Based on learnings, these snapshots should be kept in sync with the generator, not hand-edited.packages/rspeedy/plugin-react/package.json (1)
47-47: Remove or replace@upupming/type-config— package not found on npm.The dependency
@upupming/type-config: "0.0.1"does not appear to exist on the public npm registry. Installation will fail unless this is a local workspace package or private registry entry.Confirm whether this should be:
- A local workspace reference (
workspace:*or path)- Removed entirely if no longer needed
- Replaced with an alternative package
- Published to npm if intended as a scoped package
🧹 Nitpick comments (6)
packages/rspeedy/plugin-react/test/validate.test.ts (1)
188-199: Consider using inline snapshots for consistency.The new test loops use
.toThrowErrorMatchingSnapshot()while all other tests in this file use.toThrowErrorMatchingInlineSnapshot(). Inline snapshots make the expected error messages immediately visible during code review and maintain consistency with the existing test style.Example refactor for one test:
compilerOptionsKeys.forEach((compilerOptionsKey: string) => { test(compilerOptionsKey, () => { - expect(() => validateConfig({ [compilerOptionsKey]: Symbol.for('test') })) - .toThrowErrorMatchingSnapshot() + expect(() => validateConfig({ [compilerOptionsKey]: Symbol.for('test') })) + .toThrowErrorMatchingInlineSnapshot() }) })After running the tests once, Vitest will automatically populate the inline snapshot with the actual error message.
packages/rspeedy/plugin-react/test/config.test-d.ts (1)
90-102: Consider the maintainability of hard-coded length checks.The hard-coded type length assertions (38, 130, 174) will fail whenever the upstream
@upupming/type-configpackage adds or removes config options, even for non-breaking additions. This creates maintenance burden and may generate false positives.Consider whether these specific length checks provide sufficient value to justify the maintenance cost, or if the other type-level tests (extension checks, intersection checks, default field checks) already provide adequate type safety guarantees.
packages/rspeedy/plugin-react/etc/react-rsbuild-plugin.api.md (1)
8-9: New public option surfaces look aligned, but address new API Extractor warnings in source.
- The new
PluginReactLynxOptions/ReactLynxDefault*/ResolvedPluginReactLynxOptionscomposition and theLynxCompilerOptions/LynxConfigre‑exports match the PR’s “three‑layer merge” design and look consistent.- API Extractor now reports:
RequiredNotUndefinedas a forgotten export.ResolvedPluginReactLynxOptionsmissing a release tag.
To keep the public surface clean, please adjust the TS sources so that either:RequiredNotUndefinedis exported from the entry point (if it’s intended to be part of the public type toolbox), or it’s no longer referenced from a public alias; andResolvedPluginReactLynxOptionsis annotated with an explicit release tag (@public,@alpha, etc.), or marked@internalif you don’t want it public.Then re‑run
pnpm turbo api-extractor -- --localand commit the regenerated report rather than editing this file directly.As per coding guidelines, API report files should only be updated via API Extractor output.
Also applies to: 51-54, 59-80, 82-83, 97-101
packages/webpack/template-webpack-plugin/etc/template-webpack-plugin.api.md (2)
134-139: Address theae-forgotten-exportwarning forSetRequired.The API report shows a warning that
SetRequiredneeds to be exported by the entry point. While this is an internal utility type, consumers may need it to understand theResolvedLynxTemplatePluginOptionstype fully.Consider either:
- Exporting
SetRequiredfrom the entry point, or- Documenting that
ResolvedLynxTemplatePluginOptionsusestype-fest'sSetRequiredutility
103-116: NewLynxTemplateOptionsinterface needs release tag.The API report shows
ae-missing-release-tagwarning. Consider adding@publicor@alphatag to the TSDoc forLynxTemplateOptionsinterface in the source file.packages/webpack/template-webpack-plugin/src/LynxTemplatePlugin.ts (1)
228-232: Consider simplifyingSetRequiredutility type.The current implementation includes a redundant mapping. A simpler version achieves the same result:
-type SetRequired<T, K extends keyof T> = - & { - [P in keyof T]: T[P]; - } - & { [P in K]-?: T[P] }; +type SetRequired<T, K extends keyof T> = Omit<T, K> & Required<Pick<T, K>>;
| "object.groupby": "^1.0.3", | ||
| "object.pick": "^1.3.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@lynx-js/test-tools": "workspace:*", | ||
| "@lynx-js/vitest-setup": "workspace:*", | ||
| "@microsoft/api-extractor": "catalog:", | ||
| "@types/css-tree": "^2.3.11", | ||
| "@types/object.groupby": "^1.0.4", | ||
| "@types/object.pick": "^1.3.4", | ||
| "@upupming/type-config": "0.0.1", | ||
| "css-loader": "^7.1.2", | ||
| "webpack": "^5.102.0" | ||
| }, | ||
| "peerDependencies": { | ||
| "@upupming/type-config": ">=0.0.1" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if @upupming/type-config is published on npm
npm view @upupming/type-config@0.0.1 2>&1 | head -20
echo "---"
# Search for all usages of @upupming/type-config in the monorepo
rg "@upupming/type-config" --type json -A 1
echo "---"
# Check the package structure to understand the scope
git ls-files | grep -E "(package\.json|tsconfig)" | head -20Repository: lynx-family/lynx-stack
Length of output: 188
🌐 Web query:
@upupming/type-config npm package 0.0.1
💡 Result:
I couldn't find an npm package named @upupming/type-config (v0.0.1) on the public registry. I did find other @upupming scoped packages (e.g. @upupming/hexo-renderer-markdown-it-plus), which shows the scope exists. [1][2]
If you want to verify locally, run:
- npm view @upupming/type-config@0.0.1
If the package/version doesn't exist you'll get a "404 Not Found" (or no output). For general npm registry usage see the npm config/docs. [2]
Sources:
- @upupming/hexo-renderer-markdown-it-plus package page (example @upupming scope package). (npmjs.com)
- npm config / registry docs. (docs.npmjs.com)
Citations:
- 1: https://www.npmjs.com/package/%40upupming/hexo-renderer-markdown-it-plus?utm_source=openai
- 2: https://docs.npmjs.com/cli/v8/using-npm/config/?utm_source=openai
@upupming/type-config@0.0.1 does not exist on the public npm registry.
The package added as both a devDependency and peerDependency is not published on npm. If this is an external package, it must be published before the peer dependency can work. If it's an internal package, it should use the @lynx-js scope and the workspace:* protocol instead.
Verify:
- Whether @upupming/type-config is published to npm (check version 0.0.1 specifically)
- If internal to the monorepo, move it under @lynx-js scope and update references to use workspace protocol
- If the peer dependency pattern is necessary or if the package should be a regular dependency
| "@lynx-js/react-rsbuild-plugin": minor | ||
| --- | ||
|
|
||
| Use `@upupming/type-config` for Lynx configuration types. Now you can configure Lynx configurations with type safety and autocompletion in `pluginReactLynx`. |
|
Closed, we use #2052 instead |
2 participants
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.
This PR refactors PluginReactLynxOptions to adopt the
@lynx-js/type-configapproach.Key points:
Three-layer merge: LynxCompilerOptions/LynxConfig (base) + ReactLynxDefaultCompilerOptions / ReactLynxDefaultLynxConfig (defaults) + ReactLynxOptions (plugin-specific).
Checklist