fix(message): restore API compatibility and thenable chaining semantics#8504
fix(message): restore API compatibility and thenable chaining semantics#8504xjccc wants to merge 5 commits intovueComponent:feat/vaporfrom
Conversation
- add message.warn and message.useMessage compatibility surface - support object overload for typed message methods and add onClick typing/forwarding - make thenable return a Promise for chained then calls and resolve on actual close - ensure destroy/maxCount paths trigger onClose consistently - support getContainer/top/rtl config updates on mounted container - add regression tests for object overload, config, close semantics, and chained then Tested: npm --prefix packages/ui run test -- src/components/message/__tests__/index.test.ts --run Co-Authored-By: GitHub Copilot <noreply@github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the message component implementation in packages/ui to restore API compatibility with ant-design-vue’s documented surface and to fix runtime semantics around keyed updates, close behavior, and thenable chaining.
Changes:
- Adds compatibility APIs/types:
message.warn,message.useMessage, and object-overload support for type methods (e.g.message.success({ ... })). - Reworks thenable behavior so
message.xxx(...).then(...).then(...)chains correctly and resolves on actual close. - Improves close/config behavior: ensures
onCloseruns fordestroy()+maxCountoverflow, supportsgetContainermigration, and syncstop/rtlupdates.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui/src/components/message/useMessage.ts | Implements thenable resolution-on-close, keyed updates, maxCount removal semantics, config/container migration, and adds warn/useMessage. |
| packages/ui/src/components/message/types.ts | Extends typings for object overloads, thenable then signature, warn, useMessage, and onClick. |
| packages/ui/src/components/message/tests/index.test.ts | Adds regression tests for API surface, overloads, getContainer behavior, onClose consistency, thenable chaining, and timer reset behavior. |
| packages/ui/src/components/message/MessageItem.vue | Forwards onClick and adds a watcher to reset the auto-close timer on updates. |
| packages/ui/src/components/message/MessageContainer.vue | Adds RTL class binding and RTL prop plumbing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function syncContainerStyles() { | ||
| if (!messageRootEl) return | ||
|
|
||
| if (globalConfig.top != null) { | ||
| messageRootEl.style.top = | ||
| typeof globalConfig.top === 'number' ? `${globalConfig.top}px` : globalConfig.top | ||
| } | ||
|
|
||
| messageRootEl.classList.toggle('ant-message-rtl', globalConfig.rtl === true) | ||
| } | ||
|
|
||
| function ensureMounted() { | ||
| if (mounted || typeof document === 'undefined') return | ||
|
|
||
| const container = document.createElement('div') | ||
| document.body.appendChild(container) | ||
| const host = globalConfig.getContainer?.() ?? document.body | ||
| host.appendChild(container) | ||
| containerEl = container | ||
|
|
||
| containerApp = createApp(MessageContainer, { | ||
| messages, | ||
| top: globalConfig.top, | ||
| rtl: globalConfig.rtl, | ||
| onClose: (id: string) => { | ||
| removeMessage(id) | ||
| }, | ||
| }) | ||
|
|
||
| containerApp.mount(container) | ||
| messageRootEl = container.querySelector('.ant-message') as HTMLElement | null | ||
| syncContainerStyles() |
There was a problem hiding this comment.
syncContainerStyles() mutates the DOM (style.top / classList) while MessageContainer also binds :style/:class from props.top/props.rtl that are only passed once at mount. Any subsequent re-render (e.g. when messages changes) can cause Vue to patch the root element back to the stale prop values and undo config updates. Consider making top/rtl reactive props (e.g. pass a reactive config object) or remove the template bindings and rely on one source of truth.
| if (globalConfig.top != null) { | ||
| messageRootEl.style.top = | ||
| typeof globalConfig.top === 'number' ? `${globalConfig.top}px` : globalConfig.top | ||
| } | ||
|
|
||
| messageRootEl.classList.toggle('ant-message-rtl', globalConfig.rtl === true) | ||
| } |
There was a problem hiding this comment.
syncContainerStyles() only sets style.top when globalConfig.top != null, but it never clears/removes the inline top when top is later configured to null/undefined. This can leave a stale top value on the container. Consider explicitly clearing messageRootEl.style.top (or restoring the default) when globalConfig.top is unset.
| function buildMessageReturn(id: string): MessageReturn { | ||
| const destroy = () => removeMessage(id) | ||
| destroy.then = (onfulfilled, onrejected) => { | ||
| const closePromise = new Promise<void>((resolve) => { | ||
| const exists = messages.some((m) => m.id === id) | ||
| if (!exists) { | ||
| resolve() | ||
| return | ||
| } | ||
| addCloseResolver(id, resolve) | ||
| }) | ||
|
|
||
| return closePromise.then(onfulfilled, onrejected) | ||
| } | ||
| return destroy as MessageReturn |
There was a problem hiding this comment.
buildMessageReturn assigns destroy.then = (onfulfilled, onrejected) => ... on a value inferred as () => void. In TS strict mode this typically causes (1) "Property 'then' does not exist on type ..." and (2) onfulfilled/onrejected becoming implicit any due to lack of contextual typing. Consider declaring destroy as MessageReturn up-front (cast once) so the then assignment is type-safe and the parameters are typed from MessageReturn['then'].
| const moveContainer = | ||
| mounted && !!options.getContainer && containerEl && options.getContainer() !== containerEl.parentElement | ||
|
|
||
| Object.assign(globalConfig, options) | ||
|
|
||
| if (moveContainer && containerEl) { | ||
| options.getContainer?.().appendChild(containerEl) |
There was a problem hiding this comment.
In config(), options.getContainer() is invoked multiple times (in the moveContainer calculation and again when appending). If getContainer has side effects or returns a different element per call, this can cause incorrect comparisons/moves. Consider calling it once, storing the result, and using that single reference for both the comparison and the append.
| const moveContainer = | |
| mounted && !!options.getContainer && containerEl && options.getContainer() !== containerEl.parentElement | |
| Object.assign(globalConfig, options) | |
| if (moveContainer && containerEl) { | |
| options.getContainer?.().appendChild(containerEl) | |
| let nextContainer: HTMLElement | null | undefined | |
| let moveContainer = false | |
| if (mounted && containerEl && options.getContainer) { | |
| nextContainer = options.getContainer() | |
| moveContainer = !!nextContainer && nextContainer !== containerEl.parentElement | |
| } | |
| Object.assign(globalConfig, options) | |
| if (moveContainer && containerEl && nextContainer) { | |
| nextContainer.appendChild(containerEl) |
|
|
||
| syncContainerStyles() | ||
| }, | ||
| useMessage: () => [message, () => null], |
There was a problem hiding this comment.
message.useMessage() currently returns [message, () => null], so there is no contextHolder node/component to actually render in-tree. This doesn’t match the documented useMessage contract (api + contextHolder) and won’t solve the context/ConfigProvider scoping problem. Consider implementing a real hook (similar to Modal.useModal) that returns a ContextHolder component (or VNode factory) that mounts the container within the caller’s app context.
| watch( | ||
| () => props.item.args.duration, | ||
| () => { | ||
| clearTimer() | ||
| startTimer() | ||
| }, | ||
| ) |
There was a problem hiding this comment.
The timer reset watcher only tracks props.item.args.duration. When updating an existing keyed message without changing duration (e.g. only content changes), the auto-close timer will not restart, so the message may close sooner than expected after an update. Consider watching props.item.args (object identity) or another update signal so any keyed update restarts the timer, not just duration changes.
| @@ -1,5 +1,5 @@ | |||
| <template> | |||
| <div class="ant-message" :style="containerStyle"> | |||
| <div class="ant-message" :class="{ 'ant-message-rtl': rtl }" :style="containerStyle"> | |||
There was a problem hiding this comment.
ant-message-rtl is now conditionally applied, but there doesn’t appear to be any CSS in the message styles that changes layout/direction for this class. As-is, enabling rtl won’t have visible effect. Consider adding an RTL rule (e.g. direction: rtl / alignment tweaks) for .ant-message-rtl in the message styles or using a dir="rtl" attribute so the browser applies RTL layout semantics.
| <div class="ant-message" :class="{ 'ant-message-rtl': rtl }" :style="containerStyle"> | |
| <div class="ant-message" :class="{ 'ant-message-rtl': rtl }" :style="containerStyle" :dir="rtl ? 'rtl' : undefined"> |
…vior - switch container top/rtl updates to a reactive config source to avoid DOM patch races - make getContainer resolution single-call in config() before potential container move - type buildMessageReturn as MessageReturn up-front for safe then assignment - restart auto-close timer on any keyed args update (not duration-only) - add rtl semantics via dir attribute and ant-message-rtl style rule Tested: npm --prefix packages/ui run test -- src/components/message/__tests__/index.test.ts --run Co-Authored-By: GitHub Copilot <noreply@github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function handleClick(event: MouseEvent) { | ||
| props.item.args.onClick?.(event) | ||
| } |
There was a problem hiding this comment.
onClick is now forwarded via handleClick, but there isn't a regression test covering that a click on the rendered message calls the provided callback. Adding a small test for onClick would help prevent this behavior from breaking later.
- add a regression test to verify clicking the rendered message invokes onClick - locate the target notice by message text to avoid flaky selection of stale nodes Tested: npm --prefix packages/ui run test -- src/components/message/__tests__/index.test.ts --run Co-Authored-By: GitHub Copilot <noreply@github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| while (messages.length >= globalConfig.maxCount) { | ||
| removeMessage(messages[0].id) |
There was a problem hiding this comment.
The maxCount enforcement uses a while (messages.length >= globalConfig.maxCount) { removeMessage(messages[0].id) }. Since removeMessage triggers user onClose, user code can synchronously add a new message during onClose, keeping messages.length >= maxCount and causing an infinite loop. Consider removing a fixed number of pre-existing items (snapshot ids / count) to guarantee termination while still calling onClose for the removed items.
| while (messages.length >= globalConfig.maxCount) { | |
| removeMessage(messages[0].id) | |
| const maxCount = globalConfig.maxCount | |
| const currentLength = messages.length | |
| if (maxCount && currentLength >= maxCount) { | |
| const removeCount = currentLength - maxCount + 1 | |
| if (removeCount > 0) { | |
| const idsToRemove = messages.slice(0, removeCount).map((m) => m.id) | |
| idsToRemove.forEach((id) => { | |
| removeMessage(id) | |
| }) | |
| } |
| while (messages.length) { | ||
| removeMessage(messages[0].id) |
There was a problem hiding this comment.
message.destroy() removes messages via while (messages.length) removeMessage(messages[0].id). Because removeMessage invokes user onClose, onClose can synchronously create new messages, causing this loop to never terminate. To avoid hangs, iterate over a snapshot of ids (or the initial length) taken before starting removal.
| while (messages.length) { | |
| removeMessage(messages[0].id) | |
| const ids = messages.map((m) => m.id) | |
| for (const id of ids) { | |
| removeMessage(id) |
…nClose callbacks - use fixed snapshot of message IDs to remove instead of dynamic loop - ensures termination even if onClose callback adds new messages synchronously - preserves onClose semantics for all removed messages Tested: npm --prefix packages/ui run test -- src/components/message/__tests__/index.test.ts --run Co-authored-by: GitHub Copilot <noreply@github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This PR reviews and fixes the
messageimplementation in ui to improve API compatibility and runtime behavior after the SFC + CSS refactor.What Changed
message.warnaliasmessage.useMessageentrymessage.success({ content, key, duration, ... })onClicktyping and forwarding for message items.message.xxx(...).then(...).then(...)now supports proper Promise chaining.thennow resolves on actual close.onCloseis triggered consistently indestroy()andmaxCountoverflow paths.getContainermounting/migration.topandrtlupdates to mounted container.warn,useMessage)getContainerbehavioronCloseconsistencythenValidation
npm --prefix packages/ui run test -- src/components/message/__tests__/index.test.ts --runNotes