feat/buildin com mjs format #918
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/builtinComponent/vite.config.js (1)
26-28: Consider adding migration guide for breaking changeThe combination of
.mjsextension withformats: ['es']enforces ES module-only distribution. While this is a forward-looking change, it's a potential breaking change for:
- CommonJS environments using
require()- Bundlers/tools that don't handle
.mjsextensions automatically- Existing imports that hardcode the
.jsextensionConsider:
- Adding a migration guide in the PR description
- Documenting the ES modules requirement in README
- Potentially providing both
.mjsand.jsoutputs during a transition period
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/builtinComponent/vite.config.js(1 hunks)
🔇 Additional comments (1)
packages/builtinComponent/vite.config.js (1)
26-26: Verify consumer compatibility with .mjs extension
The change from .js to .mjs extension explicitly indicates an ES module format, which aligns well with formats: ['es']. However, this change might affect existing consumers.
Consider the following impacts:
- Node.js will enforce strict ES module syntax
- Import statements must use the
.mjsextension explicitly - CommonJS
require()cannot be used to import this module
Let's verify if there are any direct imports that need updating:
✅ Verification successful
The .mjs extension change is safe to proceed
The change from .js to .mjs extension is well-aligned with the package configuration and won't cause compatibility issues because:
- The package is already configured with
"type": "module"in package.json - No direct imports of the specific file extension were found in the codebase
- The devAliasPlugin references the package by directory (
packages/builtinComponent/index.js), which will still work as Node.js resolves the correct file based on package.json - The package's
mainandmodulefields in package.json point todist/index.js, which will be updated during build
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for imports of the builtin component
rg -g '!node_modules' -g '!dist' "from.*builtinComponent.*index\.js"
# Search for require statements
rg -g '!node_modules' -g '!dist' "require.*builtinComponent.*index\.js"
Length of output: 144
Script:
#!/bin/bash
# Let's try a broader search for imports of builtinComponent
rg -g '!node_modules' -g '!dist' "from.*builtinComponent"
# Search for any require statements with builtinComponent
rg -g '!node_modules' -g '!dist' "require.*builtinComponent"
# Check package.json files that might reference builtinComponent
fd "package.json" --exec grep -l "builtinComponent"
# Look for any file that might import or use builtinComponent
rg -g '!node_modules' -g '!dist' "builtinComponent" -l
Length of output: 512
Script:
#!/bin/bash
# Let's check the package.json files to understand how builtinComponent is referenced
cat packages/builtinComponent/package.json
cat packages/blockToWebComponentTemplate/package.json
# Check the devAliasPlugin to see how it uses builtinComponent
cat packages/build/vite-config/src/vite-plugins/devAliasPlugin.js
Length of output: 8883
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/builtinComponent/package.json (1)
5-6: Breaking change: Package now uses .mjs extension for ESM formatThe change from .js to .mjs aligns with modern JavaScript practices for explicit module formats. However, this is a breaking change that requires careful consideration.
Consider the following recommendations:
- Update the package documentation to clearly communicate this breaking change
- Provide migration guidelines for consumers still using CommonJS require()
- Consider using dual package hazard patterns (providing both ESM and CommonJS builds) if backward compatibility is needed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/builtinComponent/package.json(1 hunks)
🔇 Additional comments (1)
packages/builtinComponent/package.json (1)
5-6: Verify build output and consumer compatibility
Let's verify the build configuration and potential impacts.
✅ Verification successful
Build configuration is properly set for ESM output
The package is correctly configured for ESM:
- Vite config is properly set up with
formats: ['es']and outputs.mjs - Package.json correctly declares
"type": "module" - No CommonJS
require()statements found in dependent packages - Consumers are already using ESM imports
- Vue is properly externalized in the build config
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check build configuration and potential compatibility issues
# Check if vite config exists and properly configured
echo "Checking Vite config for proper .mjs output..."
rg -l "build.*lib.*fileName.*mjs" "vite.config"
# Look for potential CommonJS require statements that might break
echo "Checking for potential CommonJS require statements in the codebase..."
rg "require\(['\"]@opentiny/tiny-engine-builtin-component['\"]"
# Check if package is imported in other workspace packages
echo "Checking package usage across the workspace..."
rg "from ['\"]@opentiny/tiny-engine-builtin-component['\"]"
Length of output: 1077
Script:
#!/bin/bash
# Let's check the build configuration in the package directory
# Check if there's a vite.config or rollup.config in the package
echo "Checking build config files in package directory..."
fd "vite.config|rollup.config" packages/builtinComponent/
# Check package.json for build scripts and dependencies
echo "Checking package.json build configuration..."
cat packages/builtinComponent/package.json
# Check if there are any type definitions that need to match
echo "Checking for type definitions..."
fd "index.d.ts|types.d.ts|index.d.mts" packages/builtinComponent/
Length of output: 1225
Script:
#!/bin/bash
# Let's check the vite config to ensure proper ESM output configuration
echo "Checking vite.config.js content..."
cat packages/builtinComponent/vite.config.js
# Let's also check the actual source entry point
echo "Checking source entry point..."
fd "index.js|index.ts|index.vue" packages/builtinComponent/src/
Length of output: 1355
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
builtin 组件产物从
js格式改为mjs格式。原因：需要支持 import 导入，所以后缀需要改为 mjs
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Bug Fixes