feat(common): use the hotspot function provided by the ArkWeb JS engine to optimize the execution of defineProperties function in the common adaptation layer #3530
Conversation
…ne to optimize the execution of defineProperties function in the common adaptation layer
WalkthroughA new TypeScript declaration for an interface Changes
Sequence Diagram(s)sequenceDiagram
participant VM as vm (proxy)
participant Instance as instance (source)
participant TINY as __TINY__ (delegate)
alt __TINY__ available
VM->>TINY: createDelegate(instance, vm, filterFlags)
TINY-->>VM: Delegated properties assigned
else
VM->>Instance: Manual property definition (filtered)
Instance-->>VM: Properties assigned
end
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/vue-common/src/__longque__.d.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. packages/vue-common/src/adapter/__longque__.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. packages/vue-common/src/adapter/vue2/index.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 3
♻️ Duplicate comments (4)
packages/vue-common/src/adapter/vue3/index.ts (1)
16-16: Same dependency verification needed as Vue 2 adapter.This has the same dependency concerns as the Vue 2 adapter regarding the
../__longque__module.packages/vue-common/src/adapter/vue2.7/index.ts (3)
16-16: Same dependency verification needed.This has the same dependency concerns as the other adapters regarding the
../__longque__module.
202-205: Apply the same error handling as Vue 2 adapter.The
harmonyDefinePropertiesfunction needs the same error handling improvements suggested for the Vue 2 adapter.
207-207: Apply the same runtime validation as Vue 2 adapter.The conditional assignment needs the same runtime validation improvements suggested for the Vue 2 adapter.
🧹 Nitpick comments (3)
packages/vue-common/src/__longque__.d.ts (1)
19-20: Loosen theobjectreturn/argument typesUsing bare
objectinhibits access to properties without additional casts.
Consider something likeRecord<string, unknown>(or a generic) for better ergonomics:- createDelegate: (underlyingObject: object, initObject?: object, propertyFilterFlags?: number) => object + createDelegate: <T extends object, D extends object = {}>( + underlyingObject: T, + initObject?: D, + propertyFilterFlags?: number + ) => T & DThis keeps type information of both objects and avoids unsafe casts downstream.
packages/vue-common/src/adapter/__longque__.ts (1)
1-4: Harden the environment check & add typing
windowonly exists in browsers; usingglobalThiscovers both browsers and workers.
Adding an explicit type eliminatesanyleakage.-import { __TINY__ } +import type { ILongque } from '../__longque__' -const __TINY__ = typeof window === 'undefined' ? null : window.__Longque__ +const __TINY__: ILongque | null = + typeof globalThis !== 'undefined' && '__Longque__' in globalThis + ? (globalThis as any).__Longque__ as ILongque + : null + +export { __TINY__ }packages/vue-common/src/adapter/vue3/index.ts (1)
262-264: Document the selective optimization strategy.The selective application of optimization (using
originalDefinePropertiesfor 'setupState' and 'props', but conditionaldefinePropertiesfor 'ctx') suggests specific compatibility requirements. Please document why this distinction is necessary.Add a comment explaining the selective optimization:
const defineInstanceVm = (vm, instance) => { + // Use original implementation for setupState and props to ensure compatibility originalDefineProperties(vm, instance, 'setupState', null) originalDefineProperties(vm, instance, 'props', filter) + // Use optimized delegation for ctx when available defineProperties(vm, instance, 'ctx', filter) return vm }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/vue-common/src/__longque__.d.ts(1 hunks)packages/vue-common/src/adapter/__longque__.ts(1 hunks)packages/vue-common/src/adapter/vue2.7/index.ts(3 hunks)packages/vue-common/src/adapter/vue2/index.ts(3 hunks)packages/vue-common/src/adapter/vue3/index.ts(3 hunks)tsconfig.vue2.7.json(2 hunks)tsconfig.vue2.json(2 hunks)tsconfig.vue3.json(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*`: 用于写产品需求和开发文档的文件，需求应尽量写得简单清晰易懂。 在需求设计阶段应遵从逐步迭代的原则，早期版本可以尽量简洁。
**/*: 用于写产品需求和开发文档的文件，需求应尽量写得简单清晰易懂。
在需求设计阶段应遵从逐步迭代的原则，早期版本可以尽量简洁。
tsconfig.vue3.jsontsconfig.vue2.7.jsontsconfig.vue2.jsonpackages/vue-common/src/adapter/__longque__.tspackages/vue-common/src/__longque__.d.tspackages/vue-common/src/adapter/vue3/index.tspackages/vue-common/src/adapter/vue2.7/index.tspackages/vue-common/src/adapter/vue2/index.ts
🔇 Additional comments (6)
tsconfig.vue3.json (1)
28-29: Path looks correct – ensure publish script bundles itAdding the declaration to
includeis necessary and looks fine.
Double-check that the build/publish pipeline copiespackages/vue-common/src/__longque__.d.tsinto the final artifacts.tsconfig.vue2.7.json (1)
28-29: Include entry added – LGTMSame remark as for the Vue 3 config: verify the file ends up in the distributed typings.
tsconfig.vue2.json (1)
28-29: Typings path added – no issuesNothing further to flag.
packages/vue-common/src/adapter/vue2/index.ts (1)
6-6: ```shell
#!/bin/bashDisplay longque adapter module source and type definitions
echo "---- packages/vue-common/src/adapter/longque.ts ----"
sed -n '1,200p' packages/vue-common/src/adapter/longque.ts
echo
echo "---- packages/vue-common/src/longque.d.ts ----"
sed -n '1,200p' packages/vue-common/src/longque.d.ts</details> <details> <summary>packages/vue-common/src/adapter/vue3/index.ts (1)</summary> `252-255`: ```shell #!/bin/bash # Search for propertyFilterFlags definitions in both Vue2 and Vue3 adapters rg "propertyFilterFlags" -n -C 5 # Search for any originalDefineProperties definitions or references rg "originalDefineProperties" -n -C 3packages/vue-common/src/adapter/vue2.7/index.ts (1)
202-207: Good consistency with Vue 2 implementation.The Vue 2.7 adapter correctly uses the same filter flags and implementation pattern as Vue 2, which maintains consistency across similar Vue versions.
| interface Window { | ||
| __Longque__: ILongque | ||
| } |
There was a problem hiding this comment.
Make __Longque__ optional to match runtime reality
__Longque__ is not guaranteed to exist – your own runtime guard (typeof window === 'undefined' ? null : window.__Longque__) proves that.
Marking the property as mandatory will break type-checking when strictNullChecks is on, or when the file is imported in non-ArkWeb environments.
-interface Window {
- __Longque__: ILongque
-}
+interface Window {
+ /** Present only when running inside ArkWeb JS engine */
+ __Longque__?: ILongque
+}📝 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.
| interface Window { | |
| __Longque__: ILongque | |
| } | |
| interface Window { | |
| /** Present only when running inside ArkWeb JS engine */ | |
| __Longque__?: ILongque | |
| } |
🤖 Prompt for AI Agents
In packages/vue-common/src/__longque__.d.ts around lines 50 to 52, the Window
interface declares __Longque__ as a mandatory property, but it is not guaranteed
to exist at runtime. To fix this, make the __Longque__ property optional by
adding a question mark after its name in the interface declaration. This change
will align the type definition with the runtime reality and prevent
type-checking errors when strictNullChecks is enabled or in environments where
__Longque__ is absent.
| __TINY__.createDelegate(instance, vm, propertyFilterFlags) | ||
| } | ||
|
|
||
| const defineProperties = __TINY__ ? harmonyDefineProperties : originalDefineProperties |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider runtime validation of TINY capabilities.
The conditional assignment only checks for the existence of __TINY__ but doesn't validate if it has the required methods. Consider checking for __TINY__.createDelegate specifically.
-const defineProperties = __TINY__ ? harmonyDefineProperties : originalDefineProperties
+const defineProperties = (__TINY__ && typeof __TINY__.createDelegate === 'function') ? harmonyDefineProperties : originalDefineProperties📝 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.
| const defineProperties = __TINY__ ? harmonyDefineProperties : originalDefineProperties | |
| -const defineProperties = __TINY__ ? harmonyDefineProperties : originalDefineProperties | |
| +const defineProperties = (__TINY__ && typeof __TINY__.createDelegate === 'function') | |
| + ? harmonyDefineProperties | |
| + : originalDefineProperties |
🤖 Prompt for AI Agents
In packages/vue-common/src/adapter/vue2/index.ts at line 203, the assignment to
defineProperties only checks if __TINY__ exists but does not verify if it has
the necessary method createDelegate. Update the condition to check that __TINY__
is truthy and that __TINY__.createDelegate is a function before assigning
harmonyDefineProperties; otherwise, fallback to originalDefineProperties.
| const harmonyDefineProperties = (vm, instance) => { | ||
| const propertyFilterFlags = __TINY__.SKIP_PREFIX_UNDERSCORE | __TINY__.SKIP_PREFIX_DOLLAR | __TINY__.SKIP_CONSTRUCTOR | ||
| __TINY__.createDelegate(instance, vm, propertyFilterFlags) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the delegate creation.
The harmonyDefineProperties function calls __TINY__.createDelegate without error handling. Consider adding try-catch to gracefully fall back to the original implementation if delegation fails.
const harmonyDefineProperties = (vm, instance) => {
const propertyFilterFlags = __TINY__.SKIP_PREFIX_UNDERSCORE | __TINY__.SKIP_PREFIX_DOLLAR | __TINY__.SKIP_CONSTRUCTOR
- __TINY__.createDelegate(instance, vm, propertyFilterFlags)
+ try {
+ __TINY__.createDelegate(instance, vm, propertyFilterFlags)
+ } catch (error) {
+ console.warn('Failed to create delegate, falling back to original implementation:', error)
+ return originalDefineProperties(vm, instance, filter)
+ }
+ return vm
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/vue-common/src/adapter/vue2/index.ts around lines 198 to 201, the
harmonyDefineProperties function calls __TINY__.createDelegate without error
handling. Wrap the call to __TINY__.createDelegate in a try-catch block, and in
the catch block, implement a fallback to the original property definition method
to ensure graceful degradation if delegation fails.
使用ArkWeb JS引擎提供的热点函数，优化common适配层defineProperties函数执行
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
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
Chores
Style