Harden the proxy generator, the validation surface and the command form marker - #2446
Conversation
The generator's type map is a static with a collection initializer: no setter, no add, no fallback. A consumer with a domain type that should reach TypeScript as something other than what the map decides has nowhere to say so, and neither does one who needs an existing mapping corrected - both need a release of the generator. Add a repeatable --type-to-ts argument, fed from a TypeToTsType MSBuild item, following the pattern AssemblyToPackageMapping already established. It is consulted ahead of the built-in map, which is what lets it correct an existing mapping rather than only add an unknown one; consulted after, it could never reach a type the generator already knows. Deliberately not an attribute. A build-time concern belongs in the build file rather than on the domain type, and an attribute cannot express a mapping for a type the consumer does not own. Generated proxies are committed in consumer repositories, so the guarantee worth protecting is that a build configuring nothing generates exactly what it generated before: a spec covers configure-then-clear leaving no residue, and the 1055 existing generator specs still pass untouched.
A calendar date and a time of day are not instants, and the generated
proxy declared both as a JavaScript Date. That invents an instant the
server never sent: new Date("2026-05-12") is UTC midnight, which every
browser-local getter west of UTC reads back as the 11th, and
new Date("14:30:45") is not a date at all and yields Invalid Date.
Both types already cross the wire as their ISO-8601 string, so carrying
that string through is the faithful mapping - nothing is invented,
nothing is lost, and the timezone decision moves to the call site that
needs one instead of being made silently by the deserializer.
DateTime and DateTimeOffset are left on Date; they do denote instants.
Two specs cover it: one over the emitted TypeScript, since the defect
lives in a generated artifact nobody diffs, and one that deserializes
through the generated proxy in V8 and pins the day west of UTC.
The proxy generator's type map was recorded nowhere, so the only way to learn that a DateOnly arrives as a string - or that a Guid arrives as a Guid from @cratis/fundamentals - was to read the generator source or the generated output. Spells out the temporal cases in particular, since they are the ones where the wrong mapping is silently plausible rather than obviously broken.
The extractor DynamicInvoked FluentValidation's _errorMessageFactory during generation and wrote the returned string into the generated client validator as a literal. A factory is deferred precisely because its value is not known yet. Calling it on the build machine answers for a different process, at a different time, under ambient state that machine cannot stand in for - the culture, the clock, a tenant, a feature flag. A delegate is opaque, so there is no way to tell a factory returning a constant from one that is not, which leaves not calling it as the only guess-free move. The frozen literal was not merely stale, it was authoritative: a failing client rule short-circuits the request, so the build machine's answer is what the user sees and the server never gets asked. The rule still mirrors; only the message stays behind, and the client rule falls back to its own default. Fixed in GetCustomErrorMessage so all four generation paths - model-bound and controller-based, commands and queries - are covered at once. A fix at any call site would have covered one path and looked complete. The eager _errorMessage branch is untouched: a literal genuinely is context-free. Two existing specs pinned the old behavior and are inverted here.
The page claimed custom messages are carried over, without qualification. That is true of a literal and not of a factory, and the difference is the one a reader authoring localized messages needs to know about.
A ValidationResult carried nothing but prose to say what kind of rejection it was, so a client could only tell a retryable concurrency violation from a business-rule rejection by matching an English sentence - which breaks the moment the wording changes, and cannot distinguish the framework's sentence from an authored rule worded the same way. Adds ValidationResultReason and a Reason member defaulting to Rule, so anything the framework composes on the application's behalf has to say so. Applied to the concurrency path on all three sites - append, append many, and the aggregate root - and to the constraint path, which was equally mislabeled as an authored rule. Deliberately not State: that slot already carries FluentValidation's WithState value, so it belongs to whoever wrote the rule. Deliberately not an enum: rejections are composed in Arc, in Chronicle and in application code, and a closed set makes every new kind a breaking change for whoever switches over it. Additive on the wire and at the source level - Reason is init-only with a default, and the TypeScript parameter is optional, so a result from a server that predates this reads as Rule. The aggregate-root path had no concurrency coverage at all, which is how it came to carry the same flattening as the event-log path unnoticed.
When a validator throws, the framework substitutes CouldNotValidateMessage for that validator's entire result set. The substitute is shaped exactly like a genuine rejection - Error severity, free text, no members, no state - so a consumer could only recognize it by matching the English sentence, which is a literal out of Arc's source with no compatibility promise and no way to tell it from an authored rule worded the same. The result now carries ValidationResultReason.ValidatorFailed, so a consumer can render its own copy for the case instead. The fail-closed behavior is unchanged and deliberately so: returning a validation failure rather than propagating, logging the detail server-side, and saying nothing in the response about what went wrong are all correct. The message text is unchanged too - it is a developer diagnostic, and making it translatable would invite displaying it. ValidatorInvoker had no direct spec coverage; this adds it, including the contrast case proving an authored rejection still reads as a rule.
Use NullLogger rather than substituting the logger - logging is not specified - and split the fixture types into one file each.
CommandFormFields validated on every value change, and passed
autoServerValidate straight through - so with server validation on, each
keystroke was a POST to {route}/validate with no timer anywhere between
them. autoServerValidateThrottle governed a different effect, one gated
on all fields already being valid, so it damped a second request rather
than the per-character one.
Measured on a ten-character burst: 11 requests at the default throttle
(ten per-character plus one trailing), and 10 with the throttle raised
past the burst - the prop moved nothing. Now 1 and 0.
The per-change validation stays immediate but goes client-side only. It
is still the immediate driver of isValid, at no latency and no network
cost, and the throttled effect becomes the single server path. That
effect now also feeds silentValidationResult, so a rule only the server
can express still has the final say on isValid - just once the typing
stops rather than once per character.
This makes the code match what the documentation already promised:
"With throttle: 1 server call (after user stops typing for 500ms)".
Also tightens the existing throttle spec, whose toBeLessThanOrEqual(5)
bound is what let a round trip per keystroke hide underneath it.
XElement.Value returns the concatenated text content of an element and
its descendants, so every documentation element whose payload lives in
an attribute was silently deleted on its way into the generated proxy -
a self-closing see cref, seealso, paramref, typeparamref or see langword
has no text child at all. The prose then fused around the hole, leaving
only the two spaces that had surrounded it:
/// A <see cref="Widget"/> and a <c>gadget</c>.
-> A and a gadget.
Worse the more idiomatically the source is documented, since those are
the elements the .NET conventions ask for, and invisible either way
because the artifact is generated and nobody diffs it.
Replaces the three separate .Value reads with one element-aware walk:
a cref renders as {@link Name}, a langword and a paramref as inline
code, <c> keeps its backticks, an explicit label wins over any of them,
and anything else keeps its prose. Whitespace collapses last, so nothing
rendered away leaves a seam.
Routing all three entry points through it also fixes a property summary
spilling its source newlines and indentation straight into the JSDoc,
which only the type path had ever normalized. Visible in the TestApps
proxies regenerated here.
The pages stated that a metadata attribute declared without concurrency: true tags the appended events "without affecting concurrency control". It does affect it. The value enters the command context regardless of the flag and reaches the append regardless of the flag, and the fallback strategy then resolves its expected tail from that metadata - so the flag governs whether the command declares a scope, while the tag governs what the check is narrowed by. Corrects events.md:141 and concurrency.md:74, completes the clause concurrency.md:101 stopped one short of, and sharpens the EventForEventSourceId note at events.md:217 to say what is inherited - not one shared scope, but the declaration, applied once per target. Also corrects the rationale on the ConcurrencyScopeBuilder spec, which stated the same false inference in source. Its assertions were always right; only the reason given for them was wrong, which is what makes this a misconception rather than a stale page. Adds the spec nothing anywhere had: that a routing-only command reaches the append with no scope AND with its tag intact. Asserting the null scope alone is exactly what let the misconception survive in a repository whose specs are otherwise thorough. No behavior change. The implementation is coherent and deliberate; making a routing-only tag concurrency-inert would silently widen every existing consumer's guard.
The ownership model's remarks explained which provider wins and stopped there. What follows from winning is that the provider also decides which serialization boundary the injected read model crosses - and the three shipped providers cross entirely different ones. So a MongoDB convention pack, class-map customization, element rename or custom serializer reaches a command-side read model only where the MongoDB resolver claimed it. Chronicle and Entity Framework Core both declare, and declaring beats fallback regardless of registration order, so in an application whose read models are owned by either, it reaches none of them - while the same customization stays plainly at work on the query side, which is what makes the absence hard to notice. States it in all three places a reader might look: the ownership remarks where the precedence rule already is, and each resolver's own remarks where someone investigating a non-applying convention would go. Entity Framework Core's materialization path was recorded as unknown in the proposal this came from. It is DbContext.FindAsync through EF Core's own entity model, established here so the paragraph could name all three rather than two.
Arc coerces a query argument to the declared parameter type before validation runs, and resolves a ConceptAs<T>'s validator by the value's runtime type. So a parameter declared as the concept is validated and a raw string or Guid is not - and converting inside the body produces the concept the query wanted while skipping the rules meant to guard it, with nothing in the build, the lint step or the spec suite to notice. The shape is usually inherited rather than chosen: a query that began as a string keyed lookup keeps its parameter through every later refactor while the conversion migrates into the body as a cast, so each refactor makes it less conspicuous rather than more. Three decisions the proposal left open: - Public and internal, not public only. Query discovery registers internal methods, so an internal query is just as routable and just as unvalidated. - Warning, not error. The shape is legal and retyping is not behavior-neutral - an omitted argument deserializes to null rather than NotSet, and a strict validator becomes reachable - so a rule that breaks a warnings-as-errors build would be suppressed, not adopted. Both caveats are in the descriptor's description. - ConceptAs only, no EventSourceId arm. EventSourceId is a ConceptAs<string> and is matched anyway, which keeps a Chronicle namespace out of a package that does not know Chronicle exists. The rule also sees through the .Value on a nullable parameter, since there is no conversion from Guid? to a concept and the author has to write (RequestId)id.Value - matching only the bare reference would leave the rule silent on the shape a nullable parameter forces. Registered as an operation-block analyzer, which has no precedent in either analyzer project here - the one existing operation-based analyzer uses RegisterOperationAction for invocations. Worth a reviewer's attention for that reason.
The nine assertions in CommandResultShouldExtensions had no extension point of any kind, so a repository wanting a house rule about what a good assertion looks like could only install it by namespace shadowing - invisible at the call site, silently disarmed by moving the file, and payable once per root namespace. ICommandResultAssertionPolicy is discovered the way ICommandScenarioExtender already is, through the type discovery system, so a repository installs its policy in one visible place and it runs everywhere without a call site being edited. Three decisions the proposal left open: - Discovered, not a settable static. Per-scenario is unreachable - the assertions extend CommandResult, not the scenario - and a mutable static would be shared state across a parallel xUnit run. Discovery is process-wide but immutable, and it is the pattern this package already uses. - Strengthen only. A policy is consulted after the built-in check has passed and never when it failed, so a passing assertion can become a failure but a failing one can never become a pass. The package's own guarantees are not negotiable by a consumer. - All nine, not just ShouldHaveValidationErrors. A seam covering one assertion and not its siblings is a strange public surface, and the assertion name comes from CallerMemberName so adding one needs no second edit and no parallel vocabulary - those names are already matched as strings by Screenplay. Behavior is byte-identical with no policy installed, which the compatibility pin asserts. Cratis.Arc.Testing had no spec project and its assertions had no direct coverage at all; this adds both.
The reason discriminator introduced for the concurrency and thrown- validator paths only covered the producers those two reports named. This carries it across the rest, so a client branching on it is not silently reading Rule for a rejection no rule produced. Two categories the earlier pass did not need: - DependencyUnavailable, for a read model the pipeline could not resolve from the command's key. Raised before any rule runs, so nothing about the command's own rules was decided - which is exactly what makes it the most dangerous one to report as an ordinary rejection: neuter every rule a slice has and its specs still pass, because this rejects in their place. - MalformedRequest, for a body that could not be read or a value that could not be bound. No rule was reached either. An authored rule stays Rule and is untouched - a DataAnnotation attribute and an aggregate's own Failed() are the author's rules, not the framework's. Adds ShouldHaveValidationErrorBecauseOf, the named counterpart to ShouldHaveValidationErrors, and lists it in Screenplay's NamedRejections where matching is by name string. Asserting only that a command was rejected cannot distinguish a rule from a race, a constraint, a throw or an unresolvable dependency; this is what lets a spec pin which. FluentValidation's ErrorCode is deliberately still not carried. Doing so is one line, but it would make that library's defaults - NotEmptyValidator, NotNullValidator - a wire-visible value consumers start depending on, which is a contract Arc would then owe. WithState remains the seam for a rule that wants to name itself.
A validator whose constructor asks for a read model a spec forgot to seed is never constructed, so not one of its rules runs - and the command is rejected anyway, by the pipeline. ShouldHaveValidationErrors passed on that, so the spec was green and would have stayed green with the rule it was named after deleted. Mutation-proven in a consumer: neutering one rule left all 29 of a slice's specs passing, with that single pipeline rejection the only error present. Now that a dependency failure says so through its reason, the testing tier can tell the two apart without matching prose - which is what the proposal recorded as the reason a testing-package-only fix had been withdrawn. Only fires when the dependency failure is the whole story. A result carrying a real rule rejection alongside it has a rule that ran, so the assertion has something to be about and stands. A spec that genuinely means to assert the case says so with ShouldHaveValidationErrorBecauseOf, which the failure message names. This is a deliberate behavior change to the testing package: a spec that was passing only on a dependency failure will now fail. That is the point - it was asserting nothing.
A CommandForm child was a field only if component.displayName equaled 'CommandFormField', so any transform that set displayName unbound every field with no error and no warning - the form renders, the input accepts typing, and nothing reaches the command. react-docgen-typescript does exactly that by default, and Storybook selects it through a documented option. Adds an isCommandFormField/isCommandFormColumn static marker, checked first, with the displayName comparison kept as the fallback. The fallback is not on a deprecation path and is the point: it is what lets a version of this package interoperate with a version of a consuming package that knows only the string, in both directions. Removing it would silently unbind every field across that boundary - the exact failure the marker exists to prevent, caused by the fix. Covers the column half too. Fixing only fields would leave columns breakable by the same transform. Shipped as a static property rather than a Symbol. A Symbol resists a name-based transform more thoroughly, but it is a public-surface choice for Cratis to make, and it needs Symbol.for to survive a duplicate install - both reasons to put the question rather than answer it here. Owed, and not mine to do: the three read sites in @cratis/components. Until they move, a field surviving a displayName rewrite is recognized by CommandForm and not by CommandDialog. Nothing regresses in the meantime, because the fallback keeps both directions working. Recount for the record: Arc has 3 field read sites, 1 column read, 4 field stamps and 1 column stamp. The corrected "six field read sites" in the proposal is the total across both repositories.
The marker is a cross-package contract that nothing enforces. @cratis/components reads the field marker written here and writes the column marker read here, and neither imports the other's helper — a consuming package declares this one as a version range, so a named import would be a hard module-link error against any version in that range predating the marker. The contract is therefore carried entirely by two property names being spelled identically in two repositories. That is not hypothetical. The two packages were briefly implemented with different shapes, one a static property and the other a Symbol, and every spec in both repositories passed: both kept the displayName fallback, so nothing threw, and the marker simply stopped crossing the boundary. A field whose displayName a build transform had rewritten bound in a bare CommandForm and silently unbound inside a CommandDialog — the exact failure the marker was added to prevent, surviving the fix and invisible to every gate. Adds the spec that catches it, in both directions: a component marked the way a consuming package marks one, with its displayName then overwritten, is recognized here; and a component marked here exposes the marker under the property name that package reads, with the legacy displayName still set for one that predates the marker. Renaming either marker now reds this spec. Also corrects "recognising" to American English per .ai/rules/general.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DRCRgRvMz9N8P8NGwR34MM
Declared as a range in dependencies, which tells a package manager a copy each is acceptable. It is not: the two copies get separate converter registries and separate class objects, so a converter registered on one reaches only that one, an instanceof against the other is false, and a version pinned at the top level never reaches the nested copy - an adopted fix can land nowhere while every build stays green. A peer dependency is the mechanism for "exactly one of these in the tree", which is what this actually is. The range is deliberately wide so a patch or minor release does not force lockstep releases across repositories, and an exact devDependency keeps this repository's own build and specs pinned.
Declared as a range in dependencies, which tells a package manager a copy each is acceptable. It is not: the two copies get separate converter registries and separate class objects, so a converter registered on one reaches only that one, an instanceof against the other is false, and a version pinned at the top level never reaches the nested copy - an adopted fix can land nowhere while every build stays green. A peer dependency is the mechanism for "exactly one of these in the tree", which is what this actually is. The range is deliberately wide so a patch or minor release does not force lockstep releases across repositories, and an exact devDependency keeps this repository's own build and specs pinned.
The corrected temporal defaults and the consumer mapping seam were built independently, and their composition is the part a consumer relies on: the default has to be usable by someone who configures nothing, and replaceable by someone who wants a real calendar-date type. A default that could not be overridden would force a fork; a seam that had to be configured before the common case was correct would be a wrong default with extra steps. Documents TypeToTsType, which had no documentation at all, on the page that already carries the built-in table - so a reader sees the default and the way to change it in one place.
|
Reviewer context — not release notes. Why one PR#2442, #2443, #2444 and #2445 are all in here, so this lands as one release rather than four. Two of them overlapped and could not have been merged independently as they stood:
The decision this PR makes
The built-in default is correct without configuration; the seam is for wanting something better. A calendar date crossing as its ISO-8601 string loses nothing and cannot be rendered a day early. A consumer who wants a real
How the changes were arrived atEach was reproduced here before being fixed and mutation-proven — red before, green after, red again with the fix deleted. Three are worth attention because the evidence is unusual.
That fix makes the code match documentation it had diverged from. The routing-only concurrency claim is pinned end to end. Design decisions a maintainer may want to revisit
Verification
Pre-existing and unrelated: a Must ship togetherCratis/Components#112 carries the other half of the command form marker. A version of Components that only knows Coverage gaps closed on the way
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 116 out of 116 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Documentation/frontend/core/commands/command-result.md:199
- The import path in this example is incorrect.
@cratis/arcdoes not exportValidationResultReasonas a named export, so this snippet won’t compile for consumers. Import from the validation entrypoint (or use thevalidationnamespace export) instead.
import { ValidationResultReason } from '@cratis/arc';
The peer range replaced a "^7.16.0" dependency with "^7", which widened it by 16 minor versions rather than carrying the existing floor across. The devDependency still pins 7.16.0, so 7.16.0 is the only version the package is ever compiled or tested against, while the manifest claimed every 7.x back to 7.0.0 would do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UPD7MH4M4YuDacpjxXRaJa
@cratis/arc.react imports Constructor, field, Guid and JsonSerializer from @cratis/fundamentals across 20 files, including Bindings, IdentityProvider and the query hooks, but declared it in neither dependencies, peerDependencies nor devDependencies. It resolved only because @cratis/arc carried fundamentals as a regular dependency and the package manager hoisted it. Making that a peer on @cratis/arc removes the provider, so a consumer installing @cratis/arc.react under a resolver that does not hoist - pnpm, or Yarn with PnP - gets an unresolved import. The dependency is now declared where it is used. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UPD7MH4M4YuDacpjxXRaJa
|
Reviewer context for the two dependency commits (not release-note material): Verified locally — How the missing declaration was found — Worth a follow-up, not fixed here — the exact The |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 117 out of 117 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Documentation/frontend/core/commands/command-result.md:199
- The code sample imports
ValidationResultReasonfrom@cratis/arc, but the package root doesn’t export it as a named export (it only exposes avalidationnamespace). This import will fail for consumers; import from the validation entrypoint instead.
import { ValidationResultReason } from '@cratis/arc';
Documentation/frontend/react/command-form/custom-fields.md:83
- The caution block says that rewriting
displayNamecan unbind a field, but the implementation checks theisCommandFormFieldmarker first specifically to survive transforms that rewritedisplayName(and the paragraph right below calls out Storybook doing exactly that). This reads as internally contradictory and may mislead consumers; consider rephrasing to focus on the actual failure mode: stripping static properties (or needingdisplayNamefor interop with older packages).
:::caution
**Do not overwrite `displayName` on a command form field**, and do not strip static properties from one. A build transform that does either can unbind the field from its command — the form renders, the input accepts typing, and nothing reaches the command. There is no error and no warning.
The most likely source is Storybook's `reactDocgen: 'react-docgen-typescript'` setting, whose plugin rewrites `displayName` by default. The `isCommandFormField` marker is there to survive exactly that, so a field keeps working under it — but a transform that removes both loses the binding.
Three changes to the release path, all of them for failures that report green. Publish grouped its concurrency on github.ref with cancel-in-progress, which pull-requests.yml already had to move away from for the same reason: merging a consolidation closes every pull request whose commits it carries, each fires its own pull_request:closed event, and on a shared group the last run to start cancels the rest - including the one meant to cut the release. Keyed on the pull request number now. The release job had no guard, so a pull request closed without merging still ran it. It now runs only for a merge or a manual dispatch. A merged pull request with no semantic version label resolves should-publish to false, which skips every publish job and leaves the run green having released nothing. verify-semver-label catches the missing label before the merge, and verify-published fails the run if it happens anyway. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UPD7MH4M4YuDacpjxXRaJa
A TypeScript type is the one TypeToTsType metadata value that legitimately
contains a space - Record<string, number>, string | null, { a: number }. The
item was interpolated into the command line unquoted, so the shell split it in
two, the first half still parsed as a valid mapping with the type truncated at
the space, and the generator emitted the truncation as the property's type. No
error anywhere; the only evidence was in generated output.
Parsing bounded the entry to three fields, so an '=' inside the type stays part
of it rather than being read as the package separator - the way the neighbouring
--assembly-to-package and --namespace-root parsers already slice to the end.
An entry that cannot be used is now named instead of dropped. Ignoring one
silently produces the generator's built-in type, which looks like a working
result for a declaration that never took effect.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UPD7MH4M4YuDacpjxXRaJa
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 119 out of 119 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Documentation/frontend/core/commands/command-result.md:199
- The example imports
ValidationResultReasonfrom@cratis/arc, but the package root only exportsvalidationas a namespace object (seeSource/JavaScript/Arc/index.ts). As written, this import will fail for consumers; import from the validation entrypoint instead.
import { ValidationResultReason } from '@cratis/arc';
The lower bound of the peer range is a compatibility claim; the exact devDependency pin is the only version this package is compiled and tested against. Nothing connected the two, which is how a ^7.16.0 dependency became a ^7 peer range while the pin stayed at 7.16.0 - sixteen minor versions of claimed support that nothing exercised, and no failure until a consumer resolved one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UPD7MH4M4YuDacpjxXRaJa
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 120 out of 120 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Documentation/frontend/core/commands/command-result.md:201
- The package root (
@cratis/arc) does not exportValidationResultReasonas a named export; it only exposes avalidationmodule object. This example import will fail for consumers—import from the validation entrypoint instead.
```typescript
import { ValidationResultReason } from '@cratis/arc';
const result = await command.execute();
Source/DotNET/Testing/Commands/CommandResultAssertionPolicies.cs:49
- Uses a built-in exception type (
InvalidOperationException) in assertion infrastructure. The repo’s C# standards prefer custom exception types; this can be expressed as aCommandResultAssertionExceptionso failures are reported consistently through the testing surface.
|
@woksin I see there is a breaking change here with regards to making DateOnly and TimeOnly a string. Another thing; making @cratis/fundamentals a peer dependency. Not sure I understood the motivation for this. I would argue Fundamentals is not something one deliberately takes a dependency to, as one is not actually looking for its functionality in an application - but rather one is looking for what Arc can do and typically end up having @cratis/arc.react - which then has @cratis/arc, which then har @cratis/fundamentals as dependency. |
|
@einari Agreed on both counts being worth stopping for.
Once that releases, the two entries here become The consequence for this PR is that it should wait for that release rather than ship the string mapping. The alternative is dropping the temporal change from here and doing it in a later Arc release, which costs a second release for the same fix. I'd rather hold this one; nothing else in it is blocked. Two things in #1093 worth your eye, since they're judgement calls rather than mechanics: The peer dependency — your reasoning about intent is right: nobody sets out to depend on Fundamentals, they want what Arc does, and the chain The motivation was a different one, and I should have stated it here rather than leaving it to be inferred. Fundamentals' type registry keys on constructor identity — That said, it's a hazard a lockfile and dedupe usually handle, and the DX cost you're pointing at is real and immediate where the hazard is occasional. Happy to revert it to a normal dependency and let the guard carry that risk instead — your call, and I'd rather take your read on it than trade a concrete papercut for a rare one. |
|
@woksin Looks great in Fundamentals :) Nice having things typed. |
7.17.0 carries DateOnly and TimeOnly with converters of their own, so the generator names the type rather than falling back to a string. A string was correct - it is exactly what the server sent - but it said nothing about what it held, leaving a calendar date indistinguishable from any other string and every call site parsing it. The wire payload is unchanged, so this is the same value arriving as a type instead of as text.
|
@einari Both of your points are addressed in
The peer dependency is reverted to an ordinary dependency. Your reasoning holds, and there is a stronger argument for it than the one I gave against it: a peer does not reach the consumer transitively through It also inverts what it was meant to prevent. Duplicates matter because the converter registry keys on constructor identity — but with a plain transitive dependency and no application-level declaration there is exactly one range in the tree. Making the consumer declare a version is the only way a second, incompatible one gets in. The duplicate-install failure is not hypothetical, and this ran into it. The workspace was carrying That is worth noting for the peer-versus-dependency question rather than against it: the copies came from Cratis's own workspace disagreeing with itself, not from a consumer. Keeping the ranges aligned across our packages is the control that actually holds, and a CI check for it would catch the real cause. Happy to add one if you want it. The manifest spec that pinned the peer range is kept and retargeted at the dependency — the invariant it encodes (the range's floor must equal the version actually built against) is worth as much either way. One piece of commit hygiene I got wrong and cannot fix without a force-push: both changes landed in |
Silent validation is what decides isValid, and with autoServerValidate every run of it is a round trip. The effect that issues them re-runs on every values change, so overlapping runs are normal rather than exceptional - and whichever one resolved last was written, which makes the winner arrival order rather than issue order. A slower run describing values the form no longer holds lands after a faster run describing the current ones and overwrites it. isValid is derived from that single slot and nothing recomputes it, so the stale verdict was terminal: submit greyed out and stayed that way with every field valid and no message shown anywhere, until some unrelated interaction happened to schedule another run. It presents as "the button doesn't work", and it defeats automated UI testing of any form-driven command because a test cannot tell it apart from a genuinely invalid form. Each run now claims a token before it starts and hands it back with its result; a result a later run has already overtaken is discarded rather than applied. All three writers of that slot take part - the init effect, the per-keystroke run and the throttled server round trip - because they race each other and not only themselves, and a guard on one of them leaves the defect reachable through the other two. Nothing is cancelled, so the newest run still lands when it happens to be the slow one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XnPodg6z1KqeAgnxJpdWdz
`@cratis/arc` carries fundamentals as an ordinary dependency while
`@cratis/arc.react` still declared it as a peer, so the two packages that
ship together in the same application disagreed about whose job it is to
name a version - and only one of them had a spec saying so.
Both now declare it as a dependency, and the ranges are pinned to each
other. Letting them drift is how an application resolves one copy for
`@cratis/arc` and a different one for `@cratis/arc.react`, and a second
copy brings its own converter registry and its own `Guid` class object:
a converter registered on one is invisible to the other and `instanceof`
across them is false. Nothing throws - values simply stop being
recognized - which is why fundamentals ships a guard to report it.
The rationale on the existing spec claimed the application never imports
fundamentals. It does: the proxy generator emits `import { Guid } from
'@cratis/fundamentals'` straight into application code, and since 7.17.0
`DateOnly` and `TimeOnly` as well. The conclusion is unchanged - the
application never chose the library and has no view on its version, Arc
does, because Arc's generated output is what has to compile against it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XnPodg6z1KqeAgnxJpdWdz
The npm side moved to 7.17.0 so the proxy generator could name the `DateOnly` and `TimeOnly` types it now emits, while the NuGet pin stayed at 7.16.8. Two halves of one release sitting a version apart is a question every reader has to answer again from scratch. 7.17.0 adds nothing on the .NET side that Arc consumes - the release is the two TypeScript types and their converters - so this is parity rather than a functional bump. Restore resolves it against Chronicle 16.13.3 without conflict, and a Release build of the solution is unchanged at zero errors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XnPodg6z1KqeAgnxJpdWdz
|
Reviewer context for the three commits added on top — the rationale behind two choices that are not obvious from the diff. The validation ordering guard sits at the write point, not on
|
Chronicle 16.16.0 and Fundamentals 7.17.1 are both released, so this pull request references what is published rather than what was current when it was opened.
Chronicle 16.16.0 adds IEventStore.Registration, which every implementation outside that repository has to supply. A scenario declares its artifacts up front instead of registering them against a kernel, so NotRun is what actually happened rather than a stand-in for it.
Both specs matched the substring 'unique constraint' in the violation message. Chronicle 16.16.0 moved the offending value out of that message, and the specs failed - for a wording change, not a behaviour one. ValidationResultReason exists in this pull request for exactly this: the reason is the fact, the message is presentation and changes without notice.
Summary
One release covering the proxy generator, the command pipeline's validation surface, and the command form. Supersedes #2442, #2443, #2444 and #2445. Three changes are breaking.
Added
TypeToTsTypebuild item, declaring how a .NET type crosses the wire — consulted ahead of the built-in map, so it corrects an existing type as readily as it declares a new oneValidationResultReasononValidationResult, so a rejection can be told apart from another without matching message text, which is presentation and changes without noticeShouldHaveValidationErrorBecauseOf, for asserting why a command was rejected — asserting only that it failed passes just as readily when it failed for the wrong reasonICommandResultAssertionPolicy, a seam for applying a repository's own policy on top of the built-inCommandResultassertions, so a shared convention lives in one place instead of every specARC0015, warning when a query parameter is declared raw and converted to a concept in the method body, which skips the concept's validator so an invalid value reaches the handler as though it had been validatedmarkAsCommandFormFieldandmarkAsCommandFormColumn, so a command form field survives a build transform that rewritesdisplayName— Storybook'sreactDocgen: 'react-docgen-typescript'does that by default, which unbound every field with nothing in the output to say whybeginSilentValidationon the command form context, for a custom field to claim a token before it validates so a result a later run has overtaken is discarded rather than appliedChanged
DateOnlyandTimeOnlynow generate as theDateOnlyandTimeOnlytypes from@cratis/fundamentalsrather than asDate, which rendered a calendar date a day early west of UTC and a time of day asInvalid Date. Each has a converter of its own, so the value arrives as a type rather than as text every call site has to parse. Requires@cratis/fundamentals7.17.0autoServerValidateThrottlenow governs the per-keystroke validation request instead of a second, redundant one, so typing no longer issues an unthrottled request per keystrokesetSilentValidationResulton the command form context takes the token frombeginSilentValidationand returns whether the result was applied; called without one it behaves as it didShouldHaveValidationErrorsno longer passes when the only thing that rejected the command was a dependency the pipeline could not resolve, which let a spec report success against a pipeline that never ran@cratis/fundamentalsis declared as an ordinary dependency by both@cratis/arcand@cratis/arc.react, at the same range, so an application resolves a single copy; two copies each carry their ownGuidand converter registry and neither recognizes the other's types. The range is^7.17.0Cratis.Fundamentalsis referenced at 7.17.0, level with the npm packagesFixed
CommandFormno longer lets a validation result that a later run has overtaken decideisValid— overlapping runs resolving out of order could leave the submit button greyed out for good, with every field valid and no message shown anywhere@cratis/arc.reactdeclares its own dependency on@cratis/fundamentals, which it imports across 20 files but declared nowhere — it resolved only by hoisting through@cratis/arc