Repository navigation
[quantity-formatting] Optional unitsProvider, startup race fix, missing peers, exported prop types - #1828
Open
hl662 wants to merge 9 commits into
Open
[quantity-formatting] Optional unitsProvider, startup race fix, missing peers, exported prop types#1828hl662 wants to merge 9 commits into
hl662 wants to merge 9 commits into
Conversation
…, exported prop types, error logging, stale keys - QuantityFormatting.startup shares one in-flight promise; localization getter throws a descriptive error before startup - Add @itwin/core-common and @itwin/ecschema-metadata peers (public types already depend on them) - Export component prop types and document onFormatChange timing - Log formatter/persistence-unit failures instead of swallowing them - FormatSetSelector keys no longer shift while searching; FormatSetPanel initializes from props - Persistence unit reload keyed on unit name; remove dead code and unused imports; enable noUnusedLocals
- Guard QuantityFormatting startup completions with a generation so terminate() invalidates in-flight work; document first-call-wins localization - Tests: deferred startup race, FormatSetSelector stable keys while filtering, persistence-unit reload/logging
…elApp.quantityFormatter.unitsProvider The components follow onUnitsProviderChanged when the prop is omitted, so test-viewer drops its two copies of that wiring.
hl662
added this pull request to stack #1829
October 8, 2026 03:01
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A startup completing after termination can leave its localization namespace registered indefinitely.
1 open finding
What changed in this PR
Improves quantity-formatting initialization, provider defaults, public types, diagnostics, and cleanup.
Changes:
- Makes unit providers optional and reactive to application-level changes.
- Prevents concurrent startup races and exports component prop types.
- Adds peer dependencies, diagnostics, tests, and unused-code checks.
| File | Description |
|---|---|
.changeset/@itwin-quantity-formatting-react-package-contract.md |
Records package contract changes. |
.changeset/@itwin-quantity-formatting-react-optional-units-provider.md |
Records optional provider support. |
apps/test-viewer/src/components/quantity-formatting/FormatPropertyContextMenuItem.tsx |
Uses the default units provider. |
apps/test-viewer/src/components/quantity-formatting/FormatTabPanel.tsx |
Removes explicit provider plumbing. |
apps/test-viewer/src/components/quantity-formatting/QuantityFormatButton.tsx |
Removes duplicated provider subscription. |
packages/quantity-formatting/README.md |
Documents optional providers. |
packages/quantity-formatting/api/quantity-formatting-react.api.md |
Updates extracted API declarations. |
packages/quantity-formatting/api/quantity-formatting-react.exports.csv |
Adds exported prop types. |
packages/quantity-formatting/package.json |
Adds missing peer dependencies. |
packages/quantity-formatting/src/QuantityFormatting.ts |
Coordinates startup and localization state. |
packages/quantity-formatting/src/components/quantityformat/FormatPanel.tsx |
Supports the default provider and exports props. |
packages/quantity-formatting/src/components/quantityformat/FormatSample.tsx |
Supports defaults and logs formatter failures. |
packages/quantity-formatting/src/components/quantityformat/FormatSelector.tsx |
Exports selector props. |
packages/quantity-formatting/src/components/quantityformat/FormatSetPanel.tsx |
Initializes supplied values and exports props. |
packages/quantity-formatting/src/components/quantityformat/FormatSetSelector.tsx |
Stabilizes fallback keys and exports props. |
packages/quantity-formatting/src/components/quantityformat/QuantityFormatPanel.tsx |
Resolves providers and optimizes unit loading. |
packages/quantity-formatting/src/components/quantityformat/internal/FractionDash.tsx |
Removes an unused handler. |
packages/quantity-formatting/src/components/quantityformat/internal/StationBaseFactor.tsx |
Removes an unnecessary type escape. |
packages/quantity-formatting/src/components/quantityformat/internal/misc/DecimalPrecision.tsx |
Removes an unused import. |
packages/quantity-formatting/src/components/quantityformat/internal/misc/DecimalSeparator.tsx |
Removes an unused import. |
packages/quantity-formatting/src/components/quantityformat/internal/misc/FractionPrecision.tsx |
Removes an unused import. |
packages/quantity-formatting/src/components/quantityformat/internal/misc/SignOption.tsx |
Removes an unused import. |
packages/quantity-formatting/src/components/quantityformat/internal/misc/StationSizeSelector.tsx |
Removes an unused import. |
packages/quantity-formatting/src/components/quantityformat/internal/useUnitsProvider.ts |
Adds provider resolution and subscription. |
packages/quantity-formatting/src/quantity-formatting-react.ts |
Exports public prop types. |
packages/quantity-formatting/src/test/QuantityFormatting.test.ts |
Tests startup lifecycle behavior. |
packages/quantity-formatting/src/test/_Setup.test.ts |
Removes an unused test import. |
packages/quantity-formatting/src/test/quantityformat/FormatSample.test.tsx |
Removes an unused import. |
packages/quantity-formatting/src/test/quantityformat/FormatSetSelector.test.tsx |
Tests stable filtered selection. |
packages/quantity-formatting/src/test/quantityformat/QuantityFormatPanel.test.tsx |
Tests reset, loading, and logging behavior. |
packages/quantity-formatting/src/test/quantityformat/internal/DecimalSeparator.test.tsx |
Removes an unused import. |
packages/quantity-formatting/src/test/quantityformat/internal/FormatPrecision.test.tsx |
Removes an unused import. |
packages/quantity-formatting/src/test/quantityformat/internal/FractionDash.test.tsx |
Removes an unused import. |
packages/quantity-formatting/src/test/quantityformat/internal/KeepDecimalPoint.test.tsx |
Removes an unused import. |
packages/quantity-formatting/src/test/quantityformat/internal/KeepSingleZero.test.tsx |
Removes an unused import. |
packages/quantity-formatting/src/test/quantityformat/internal/ShowTrailingZeros.test.tsx |
Removes an unused import. |
packages/quantity-formatting/src/test/quantityformat/internal/SignOption.test.tsx |
Removes unused test imports. |
packages/quantity-formatting/src/test/quantityformat/internal/StationBaseFactor.test.tsx |
Removes an unused import. |
packages/quantity-formatting/src/test/quantityformat/internal/ThousandsSeparator.test.tsx |
Removes an unused import. |
packages/quantity-formatting/src/test/quantityformat/internal/ZeroEmpty.test.tsx |
Removes an unused import. |
packages/quantity-formatting/src/test/quantityformat/internal/useUnitsProvider.test.ts |
Tests provider resolution and updates. |
packages/quantity-formatting/src/test/quantityformat/panels/AzimuthPanel.test.tsx |
Removes an unused import. |
packages/quantity-formatting/src/test/quantityformat/panels/BearingPanel.test.tsx |
Removes unused test imports. |
packages/quantity-formatting/src/test/quantityformat/panels/DecimalPanel.test.tsx |
Removes an unused import. |
packages/quantity-formatting/src/test/quantityformat/panels/FractionalPanel.test.tsx |
Removes an unused import. |
packages/quantity-formatting/src/test/quantityformat/panels/RatioPanel.test.tsx |
Removes an unused import. |
packages/quantity-formatting/src/test/quantityformat/panels/ScientificPanel.test.tsx |
Removes an unused import. |
packages/quantity-formatting/src/test/quantityformat/panels/StationPanel.test.tsx |
Removes an unused import. |
packages/quantity-formatting/tsconfig.json |
Enables unused-code checks. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
…ration that completes after terminate Keeps the registration when a newer startup reuses the same localization instance.
Member
Why should they be exported? |
grigasp
reviewed
Oct 9, 2026
Comment on lines
62
to
82
| private static async initialize(localization: Localization, generation: number): Promise<void> { | ||
| // Assigned before awaiting so components rendered while the namespace loads keep working. | ||
| QuantityFormatting._localization = localization; | ||
| try { | ||
| await localization.registerNamespace(QuantityFormatting._i18nNamespace); | ||
| } catch (error) { | ||
| // Allow a retry, unless terminate() already reset the state for a newer startup. | ||
| if (generation === QuantityFormatting._generation) { | ||
| QuantityFormatting._startupPromise = undefined; | ||
| QuantityFormatting._localization = undefined; | ||
| } | ||
| throw error; | ||
| } | ||
| if (generation === QuantityFormatting._generation) { | ||
| QuantityFormatting._isInitialized = true; | ||
| return; | ||
| } | ||
| // terminate() ran while registering. Undo this registration unless a newer startup uses the same instance. | ||
| if (localization !== QuantityFormatting._localization) | ||
| localization.unregisterNamespace(QuantityFormatting._i18nNamespace); | ||
| } |
Member
There was a problem hiding this comment.
why do you need all this? why would anyone call startup more than once?
Contributor
Author
There was a problem hiding this comment.
fair, there was a bunch of state management I was overthinking and overenginnering when planning this out...
Okay I simplified it so we just leverage the _startupPromise, and if startup is called again while it runs (albeit i don't see why this would happen if startup is quick) then that will reuse the current Promise
| return IModelApp.quantityFormatter.onUnitsProviderChanged.addListener(update); | ||
| }, [unitsProvider]); | ||
|
|
||
| return unitsProvider ?? defaultProvider ?? IModelApp.quantityFormatter.unitsProvider; |
Member
There was a problem hiding this comment.
This is a bit confusing as defaultProvider may be either based on explicit unitsProvider or IModelApp.quantityFormatter.unitsProvider, and we're mixing all three of them together here. AI suggests there's also an edge case, where we'd return a stale value:
| Step | Explicit prop | App default | Cached default | Hook returns |
|---|---|---|---|---|
| Initial render | undefined |
A | A | A |
| Supply explicit provider | Custom | A | A | Custom |
| App replaces its default | Custom | B | A | Custom |
| Remove explicit provider | undefined |
B | A | A, stale |
| Effect updates state | undefined |
B | B | B |
I would consider something like this:
function subscribeToUnitsProvider(onStoreChange: () => void) {
return IModelApp.quantityFormatter.onUnitsProviderChanged.addListener(
onStoreChange,
);
}
function skipSubscription() {
return () => undefined;
}
export function useUnitsProvider(
unitsProvider?: UnitsProvider,
): UnitsProvider {
return React.useSyncExternalStore(
unitsProvider ? skipSubscription : subscribeToUnitsProvider,
() => unitsProvider ?? IModelApp.quantityFormatter.unitsProvider,
);
}
Contributor
Author
There was a problem hiding this comment.
Oh, I used useSyncExternalStore in a new stratakit impl of this UI 😄 But yes you made a good point, i've aligned to your suggestion
…ternalStore for the default units provider - startup shares one in-flight initialization and allows a retry after failure; drop the generation guard and late-registration cleanup - useUnitsProvider reads the app provider through useSyncExternalStore, so removing an explicit provider never renders a stale default
Contributor
Author
They were already part of the public signatures (FormatSelector: React.FC, etc.), but consumers couldn’t import them by name. API Extractor listed them as non-exported. So exporting them matches property-grid, which does exports its component props types. I could revert it, but I figured it could help? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
The package had cleanup items from the April refactor backlog that affect consumers:
startup()calls could race.unitsProviderand track changes to it themselves.This PR fixes those, makes
unitsProvideroptional (it defaults toIModelApp.quantityFormatter.unitsProvider), and turns on unused-code checks. Minor release.Stacked on #1827.
What
QuantityFormatting.startup/terminatestartup()calls registered the namespace twice, and aterminate()during startup could corrupt a later startup. Calls now share one in-flight startup, a counter thatterminate()bumps makes old startups no-ops, a failed startup can be retried, and readinglocalizationbefore startup throws a clear error.unitsProvideroptional onQuantityFormatPanel/FormatPanel/FormatSampleIModelApp.quantityFormatter.unitsProviderand re-subscribe toonUnitsProviderChangedby hand. The components now do that by default; an explicit provider still wins. Supported core stays at^5.3.internal/useUnitsProvider.ts(new)@itwin/core-common,@itwin/ecschema-metadata(^5.3.0)*PropstypesQuantityFormatPanelonFormatChangefires.FormatSampleFormatSetSelectorFormatSetPaneltsconfig.jsonnoUnusedLocals/noUnusedParametersunitsProvider.Tests
New and updated test coverage
QuantityFormatting.test.ts— concurrent startupstartup()is called in parallel.QuantityFormatting.test.ts— localization before startupQuantityFormatting.test.ts— retry after failureQuantityFormatting.test.ts— startup completing after terminateuseUnitsProvider.test.ts— default / follows app / explicit winsonUnitsProviderChanged.FormatSetSelector.test.tsx— stable keys while filteringQuantityFormatPanel.test.tsx— persistence unit reload / loggingQuantityFormatPanel.test.tsx— prop change resets editsformatDefinitiondiscards pending edits without saving.Validation
tsc,pnpm lint,pnpm test(204/204),pnpm build,extract-api,check-extractions, the test-viewer type-check, and thelearning-snippetsquantity-formatting tests all pass.Nambot 🤖 (powered by claude-opus-5.5)