fix: blockCompile get correct version #964
Conversation
WalkthroughThis pull request introduces modifications across multiple packages to enhance block component loading and caching mechanisms. Key changes include updating the Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Nitpick comments (4)
packages/canvas/render/src/render.js (3)
260-291: Consider concurrency or re-entrancy in block loading
If multiple calls toloadBlockComponentfor the same block occur simultaneously, you might get concurrent fetches. Also consider the security implications of dynamically importing code from external sources.
294-294: Global scope usage caution
AttachingloadBlockComponenttowindowcan open it to external tampering or naming collisions. Consider scoping or restricting usage if possible.
300-302: Clearing all block cache
removeBlockCompsCachefully clearsblockComponentsBlobUrlMap. If you need more granular cache invalidation, a per-block approach might be beneficial.packages/block-compiler/src/index.ts (1)
89-89: No longer using dynamic import
Switching toasync () => window.${globalGetterName}('${fileName}')removes code splitting benefits ofimport(). Check if this impacts performance or bundling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/block-compiler/src/index.ts(2 hunks)packages/canvas/container/src/container.js(1 hunks)packages/canvas/render/src/RenderMain.js(2 hunks)packages/canvas/render/src/render.js(1 hunks)packages/plugins/materials/src/composable/block-compile.js(4 hunks)packages/plugins/materials/src/meta/block/index.js(1 hunks)packages/plugins/materials/src/meta/block/src/BlockVersionSelect.vue(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/canvas/container/src/container.js
🔇 Additional comments (11)
packages/plugins/materials/src/composable/block-compile.js (6)
4-4: Initialize a global Map for block versions
This approach is fine if you do not require reactivity. Otherwise, consider storing it in a reactive store.
67-71: Lazy loading block versions
This logic ensures the version map is only fetched when needed, which can be more performant.
80-81: Graceful handling of missing version
If blockVersionMap.get(name) doesn't match any history item, the fallback is to blockItem?.content. This is a sensible fallback approach.
86-86: Fallback to default block content
Using the historical version if available, otherwise falling back to blockItem?.content, provides a clear path for version resolution.
49-50: Confirm generatePageCode usage of blockRelativePath
Ensure blockRelativePath: './' is recognized by generatePageCode and has the intended effect.
✅ Verification successful
Let me gather more information about the generatePageCode implementation and its usage of blockRelativePath.
blockRelativePath: './' is correctly used and recognized by the code generator
The verification shows that blockRelativePath is a well-established configuration option in the Vue code generator system:
- It's used in multiple parts of the codebase, particularly in the Vue SFC (Single File Component) generation
- The default value is '../components/' but can be overridden, as done in this case with './'
- The parameter is used for:
- Generating import statements for blocks
- Resolving paths when generating template code
- Processing block components in the SFC generator
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
rg -A 5 'function generatePageCode'
Length of output: 491
Script:
#!/bin/bash
# Search for generatePageCode implementation
ast-grep --pattern 'function generatePageCode($_) {
$$$
}'
# Search for interface or type definitions related to generatePageCode
rg -A 10 'interface.*generatePageCode|type.*generatePageCode'
# Search for blockRelativePath usage
rg -B 2 -A 2 'blockRelativePath'
Length of output: 6081
29-32: Verify that all references to this updated signature are correct
Previously, updateBlockCompileCache accepted a parameter. If old code still passes an argument, it might break. Please confirm there are no leftover references that expect a parameter.
✅ Verification successful
No issues found with updateBlockCompileCache usage
Based on the codebase analysis, the only reference to updateBlockCompileCache() is in BlockVersionSelect.vue, and it's already being called without any parameters, which matches the new signature. No other references were found in the codebase that might be passing arguments to this function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
rg "updateBlockCompileCache\("
Length of output: 157
Script:
#!/bin/bash
# Let's check the git history to see the previous signature and its usage
git log -L :updateBlockCompileCache:packages/plugins/materials/src/composable/block-compile.js
Length of output: 0
Script:
#!/bin/bash
# Let's also check for any other potential references using ast-grep
ast-grep --pattern 'updateBlockCompileCache($_)'
Length of output: 50
Script:
#!/bin/bash
# Let's also check the content of BlockVersionSelect.vue to see how it's used
cat packages/plugins/materials/src/meta/block/src/BlockVersionSelect.vue
Length of output: 4444
packages/canvas/render/src/render.js (1)
296-298: Asynchronous component loading
Using defineAsyncComponent(async () => loadBlockComponent(name)) aligns well with your caching in blockComponentsBlobUrlMap. This is a clean design.
packages/plugins/materials/src/meta/block/index.js (1)
3-10: New fetchGroupBlocksByIds usage
Adding fetchGroupBlocksByIds expands functionality. Verify that it correctly handles error states and large data sets.
packages/plugins/materials/src/meta/block/src/BlockVersionSelect.vue (1)
91-91: Revisiting the removed parameter
Previously, updateBlockCompileCache accepted a fileName. Confirm that switching to a no-parameter call does not break partial invalidation logic or other references.
packages/block-compiler/src/index.ts (1)
61-61: Updated default global getter name
globalGetterName now defaults to 'loadBlockComponent'. Ensure all references to the old 'getBlockComponentBlobUrl' are removed or updated.
packages/canvas/render/src/RenderMain.js (1)
508-508: Confirm external usage references to avoid broken imports.
Exporting removeBlockCompsCache is fine, but ensure that any dependent code or documentation referencing removeBlockComps or the old name removeBlockCompsCacheByName is updated accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/plugins/materials/src/composable/block-compile.js (1)
8-23:⚠️ Potential issueAdd error handling for API calls
The function makes multiple API calls without error handling, which could lead to silent failures.
Apply this diff to add proper error handling:
const getAllGroupBlocks = async () => { const { fetchGroups, fetchGroupBlocksByIds } = getMetaApi('engine.plugins.materials.block') const appId = getMetaApi(META_SERVICE.GlobalService).getBaseInfo().id - const groups = await fetchGroups(appId) + try { + const groups = await fetchGroups(appId) + if (!groups?.length) { + console.warn('No block groups found') + return + } const groupIds = groups.map((groupItem) => groupItem.id) const blocks = await fetchGroupBlocksByIds({ groupIds }) for (const blockItem of blocks) { if (blockItem?.content?.fileName && blockItem?.current_version) { blockVersionMap.set(blockItem.content.fileName, blockItem.current_version) } } + } catch (error) { + console.error('Failed to fetch block groups:', error) + throw error + } }
🧹 Nitpick comments (3)
packages/plugins/materials/src/composable/block-compile.js (3)
4-6: Consider implementing cache size limits and cleanup strategyThe module-level Maps (
blockVersionMapandblockCompileCache) could potentially grow unbounded. Consider implementing:
- Maximum cache size limits
- Least-recently-used (LRU) eviction policy
- Periodic cleanup mechanism
7-8: Translate comments to English for better accessibilityConsider translating the Chinese comment "获取所有区块分组下的所有区块" to English for better international collaboration.
-// 获取所有区块分组下的所有区块 +// Fetch all blocks from all block groups
Line range hint
67-81: Consider splitting getBlockByName into smaller functionsThe function handles multiple responsibilities:
- Version map management
- Block schema fetching
- Version comparison
- Resource management
Consider splitting these into separate functions for better maintainability and testing.
Example refactor:
const ensureVersionMapPopulated = async () => { if (blockVersionMap.size === 0) { await getAllGroupBlocks() } } const findHistorySchema = (blockItem, version) => { return blockItem?.histories?.find?.( (historyItem) => historyItem?.version === version ) } const getBlockByName = async (name) => { await ensureVersionMapPopulated() const blockItem = (await fetchBlockSchema(name))?.[0] if (!blockItem) return const historyVersion = blockVersionMap.get(name) const historySchema = findHistorySchema(blockItem, historyVersion) // ... rest of the function }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/materials/src/composable/block-compile.js(4 hunks)
🔇 Additional comments (2)
packages/plugins/materials/src/composable/block-compile.js (2)
29-32: Verify the impact of complete cache clearing
The function now clears all caches instead of targeted clearing by name. This could impact performance if called frequently.
49-58: Verify compatibility with blockCompiler consumers
The blocksSourceCode structure has changed from an array to an object. This could be a breaking change for consumers of the compiled result.
✅ Verification successful
Let me gather more information about the blockCompiler implementation and its usage.
Let me check the implementation of the block compiler from the external package.
The change appears safe as blockCompiler expects an array
From the code analysis, we can see that blockCompiler is imported from @opentiny/tiny-engine-block-compiler and the code explicitly wraps the blocksSourceCode object in an array when passing it to blockCompiler([blocksSourceCode], ...). This means the change from array to object for blocksSourceCode is safe as the array structure is maintained at the point of interaction with blockCompiler.
Additionally, this is the only usage of blockCompiler in the codebase, so there are no other consumers to consider.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of blockCompiler to verify compatibility
ast-grep --pattern 'blockCompiler($_, $_)'
Length of output: 332
Script:
#!/bin/bash
# Check the blockCompiler implementation
ast-grep --pattern 'function blockCompiler($_, $_) {
$$$
}'
# Also check for const/let/var declarations
ast-grep --pattern 'const blockCompiler = $_'
# Search for any other references to blockCompiler
rg "blockCompiler" -A 3
Length of output: 864
Script:
#!/bin/bash
# Check if the compiler package is available in the repository
fd "@opentiny/tiny-engine-block-compiler"
# Check package.json for this dependency
rg "@opentiny/tiny-engine-block-compiler" -A 3 "package.json"
Length of output: 101
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/canvas/render/src/render.js (1)
276-276:⚠️ Potential issueFix style element query selector issue
The
querySelectoris called before the ID is set on the style element, which will always return null on the first load.Move the query selector after setting the ID:
- const stylesheet = document.querySelector(`#${fileName}`) + const newStylesheet = document.createElement('style') + newStylesheet.setAttribute('id', fileName) + const stylesheet = document.querySelector(`#${fileName}`)
🧹 Nitpick comments (1)
packages/canvas/render/src/render.js (1)
289-292: Enhance error handling with loggingConsider adding error logging to help with debugging issues in production.
} catch (error) { + console.error(`Failed to load block component ${name}:`, error); + globalNotify({ + type: 'error', + title: `Failed to load block: ${name}`, + message: error.message + }); return h(BlockLoadError, { name }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/block-compiler/src/index.ts(2 hunks)packages/canvas/render/src/render.js(1 hunks)
🔇 Additional comments (1)
packages/block-compiler/src/index.ts (1)
61-61: LGTM: Function parameter and implementation updated correctly
The changes align well with the new block loading mechanism, updating both the default parameter and the async component definition consistently.
Let's verify that no old references to 'getBlockComponentBlobUrl' remain:
Also applies to: 89-89
✅ Verification successful
The remaining reference to getBlockComponentBlobUrl is in a development-only file
The only remaining reference to getBlockComponentBlobUrl is in packages/block-compiler/src/dev.ts, which is a development/test file used for rendering sample blocks. This is not part of the main implementation and doesn't affect the production code. The change in the main implementation (src/index.ts) is consistent and complete.
packages/block-compiler/src/dev.ts: Contains a development-only reference that can be updated separately if needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old function name
rg "getBlockComponentBlobUrl"
Length of output: 114
Script:
#!/bin/bash
# Let's check the dev.ts file to understand the context of the remaining reference
cat packages/block-compiler/src/dev.ts
Length of output: 2107
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/canvas/render/src/render.js (2)
266-266: Add type checking for controller response.The response from
getBlockByNameis used without type validation, which could lead to runtime errors if the response is malformed.Add type validation:
const blocksBlob = await getController().getBlockByName(name) +if (!blocksBlob || typeof blocksBlob !== 'object') { + throw new Error(`Invalid response from getBlockByName for ${name}`) +}
295-295: Document and type-guard the global API.Exposing functions on the global window object without type declarations or documentation makes the API harder to discover and use safely.
Consider adding TypeScript declarations and JSDoc:
+/** + * Loads a block component by name and caches its blob URL. + * @param {string} name - The name of the block component to load + * @returns {Promise<any>} The loaded component module + * @throws {Error} If the component cannot be loaded + * @global + */ window.loadBlockComponent = loadBlockComponent + +// Add to a separate .d.ts file: +declare global { + interface Window { + loadBlockComponent(name: string): Promise<any> + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/canvas/render/src/render.js(1 hunks)
🔇 Additional comments (2)
packages/canvas/render/src/render.js (2)
276-276:
Fix stylesheet selector to match the ID attribute.
The querySelector uses #${fileName} but the ID is set without any prefix in setAttribute. This could lead to incorrect stylesheet selection.
Apply this fix:
-const stylesheet = document.querySelector(`#${fileName}`)
+const stylesheet = document.querySelector(`style[id="${fileName}"]`)Likely invalid or redundant comment.
289-292: 🛠️ Refactor suggestion
Add cleanup in error handling.
When an error occurs during block loading, any styles that were already injected remain in the DOM. This could lead to style pollution.
Apply this enhancement:
} catch (error) {
+ // Clean up any styles that were injected before the error
+ blockComponentsBlobUrlMap.forEach((_, fileName) => {
+ const stylesheet = document.querySelector(`style[id="${fileName}"]`)
+ stylesheet?.remove()
+ })
+ blockComponentsBlobUrlMap.clear()
// 加载错误提示
return h(BlockLoadError, { name })
}Likely invalid or redundant comment.
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
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
Release Notes
New Features
fetchGroupBlocksByIds, enhancing module capabilities.getAllGroupBlocks, for fetching block groups and their associated blocks.Improvements
User Experience