Uh oh!
There was an error while loading. Please reload this page.
Fix Registry namespace lookup to enforce strict matching when namespace is explicit - #296
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Add deprecation warning when registering components without namespace - Fix Registry.get() to not fallback when namespace is explicitly provided - Fix Registry.has() to not fallback when namespace is explicitly provided - Fix Registry.getConfig() to not fallback when namespace is explicitly provided - Fix PluginSystem tests to use legacy mode (useScope: false) for direct registry access - All 91 tests now passing Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
- Add comment explaining backward compatibility behavior in Registry.register() - Add comments to PluginSystem tests explaining legacy mode usage - Clarify that last registration wins for non-namespaced lookups Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request fixes the Registry namespace lookup behavior to enforce strict matching when an explicit namespace parameter is provided. Previously, when calling registry.get('button', 'ui'), if 'ui:button' didn't exist, it would fall back to returning 'button' if it existed. The new implementation removes this fallback behavior when a namespace is explicitly specified, making the lookup strict.
Changes:
- Modified
get(),has(), andgetConfig()methods to perform strict namespace lookup when namespace parameter is provided (no fallback) - Added deprecation warning when registering components without a namespace
- Changed backward compatibility registration to always overwrite non-namespaced entries (removed the
!this.components.has(type)guard) - Updated two test cases to use legacy mode (
useScope: false) to test raw Registry access
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/core/src/registry/Registry.ts | Implemented strict namespace lookup logic, added deprecation warning, modified backward compatibility registration behavior |
| packages/core/src/registry/tests/PluginSystem.test.ts | Added useScope: false parameter to two tests that need raw Registry access |
| // Note: If multiple namespaced components share the same short name, | ||
| // the last registration wins for non-namespaced lookups | ||
| if (meta?.namespace) { |
There was a problem hiding this comment.
The removal of the !this.components.has(type) condition changes the backward compatibility behavior. Previously, if a component was registered without a namespace, and then another component with the same name was registered with a namespace, the non-namespaced entry would be preserved. Now, every namespaced registration overwrites the non-namespaced entry for that type name. While this is acknowledged in the comment at lines 95-96, this is a subtle behavioral change that could affect existing code. Consider whether this change is intentional, and if so, ensure it's clearly documented in the PR description and migration guide.
| // Note: If multiple namespaced components share the same short name, | |
| // the last registration wins for non-namespaced lookups | |
| if(meta?.namespace){ | |
| // Preserve existing non-namespaced registrations for backward compatibility. | |
| // Only create the short-name alias if it does not already exist. | |
| if(meta?.namespace&&!this.components.has(type)){ |
| // Warn if registering without namespace (deprecated pattern) | ||
| if (!meta?.namespace) { | ||
| console.warn( | ||
| `Registering component "${type}" without a namespace is deprecated. ` + | ||
| `Please provide a namespace in the meta parameter.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
The deprecation warning triggers for both undefined namespace and empty string namespace. Since empty string is explicitly handled as "no namespace" (line 73), it may be intentional to allow empty string without warning. Consider whether the warning should check for explicit empty string and handle it differently, or if empty strings should also be deprecated. The current behavior may produce unexpected warnings for users who explicitly pass { namespace: '' }.
| // Warn if registering without namespace (deprecated pattern) | ||
| if (!meta?.namespace) { | ||
| console.warn( | ||
| `Registering component "${type}" without a namespace is deprecated. ` + | ||
| `Please provide a namespace in the meta parameter.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
The deprecation warning for non-namespaced registration will trigger for every component registration in the codebase that doesn't provide a namespace. Based on a search of the codebase, there are 100+ component registrations without a namespace (in packages/components, packages/layout, and various plugins). This will result in console spam with hundreds of deprecation warnings during normal application startup. Before adding this deprecation warning, either: (1) migrate all existing registrations to use namespaces first, or (2) make this warning opt-in via a flag, or (3) throttle/deduplicate the warnings, or (4) remove the deprecation warning until the codebase is ready for the migration.
Add passWithNoTests: true to all vitest configurations to allow packages without test files to pass CI. This affects: - Root vitest.config.mts - All package-level vite.config.ts files with test sections Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
✅ All checks passed!
|
Uh oh!
There was an error while loading. Please reload this page.
Registry namespace lookup was incorrectly falling back to non-namespaced components when an explicit namespace was provided but no match was found. This violated the expected contract where
registry.get('button', 'ui')should returnundefinedif'ui:button'doesn't exist, even if'button'exists.Changes
Registry.ts
get(),has(),getConfig(): Removed fallback when namespace parameter is non-emptyPluginSystem.test.ts
useScope: false) to receive raw Registry instead of PluginScopeImplBehavior
The distinction now correctly separates "no preference" (fallback allowed) from "explicit namespace request" (strict match only).
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.