Allow set from loaders#600
Conversation
|
I'll add |
WalkthroughThis update refactors context parameter handling by introducing a unified Changes
Sequence Diagram(s)sequenceDiagram
participant EventProcessor
participant UserContext
participant Loader
participant EntityStore
EventProcessor->>UserContext: Build contextParams (isPreload, shouldSaveHistory, ...)
UserContext->>Loader: getLoaderArgs(contextParams)
Loader->>EntityStore: getOrCreate/set/deleteUnsafe (methods depend on isPreload)
Note right of Loader: If isPreload, set is a no-op and logger is disabled
Loader->>UserContext: Return results
UserContext->>EventProcessor: Handler/Loader output
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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: 1
🔭 Outside diff range comments (2)
codegenerator/cli/templates/static/codegen/src/UserContext.res (1)
39-46: Effect context still logs during preload
effectTrapsalways returnsLogging.getUserLogger, socontext.effect()calls inside loaders will write logs even whencontext.isPreload === true.
This violates the contract documented inTypes.ts.hbs(“logs are ignored during preload”).- | "log" => params.eventItem->Logging.getUserLogger->Utils.magic + | "log" => + (params.isPreload ? Logging.noopLogger : params.eventItem->Logging.getUserLogger) + ->Utils.magicApply the same guard as in
loaderTrapsto keep the preload pass truly silent.codegenerator/cli/templates/static/codegen/src/EventProcessing.res (1)
197-223: Silent exception swallow may hide real failures
runBatchLoadersOrThrowintentionally ignores both returned values and any synchronous exceptions from the first preload run.
While this avoids interrupting the batch, it can also mask programming errors inside loaders.Consider at least logging the caught exceptions with the root logger (maybe behind a debug flag) so that silent failures are still diagnosable.
🧹 Nitpick comments (4)
codegenerator/cli/npm/envio/src/Logging.res (1)
171-177: Consider exposing full logger interface
noopLoggeromitstraceandfatal, although those helpers exist for the real logger.
Adding them keeps the public shape identical and avoids accidentalundefinedaccess when a caller forwards a logger.codegenerator/cli/templates/dynamic/codegen/src/Types.res.hbs (1)
20-23: Minor style inconsistency inentityLoaderContextOther functions in this record omit parentheses around single arguments (
set: 'entity => unit).
For consistency, drop the parentheses ingetOrCreate:- getOrCreate: ('entity) => promise<'entity>, + getOrCreate: 'entity => promise<'entity>,Purely cosmetic but keeps generated code uniform.
codegenerator/cli/templates/static/codegen/src/UserContext.res (1)
213-283: Consider documentinggetOrCreatesemantics in preloadWhen
isPreloadis true,setis a no-op, sogetOrCreate()will return the freshly-constructed entity but not persist it.
A second call inside the same preload pass will still returnNone, which can confuse loader authors.Recommend adding a short note in the TS/Res templates (and maybe in docs) clarifying that
getOrCreateonly mutates state on the non-preload pass.codegenerator/cli/templates/dynamic/codegen/src/Types.ts.hbs (1)
19-25: Tiny wording nit“During preload entities aren't set, logs are ignored and exceptions are silently swallowed.”
Consider rephrasing “silently swallowed” → “captured and ignored” to emphasise they are still observed internally.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
codegenerator/cli/npm/envio/src/Logging.res(1 hunks)codegenerator/cli/templates/dynamic/codegen/src/Types.res.hbs(1 hunks)codegenerator/cli/templates/dynamic/codegen/src/Types.ts.hbs(2 hunks)codegenerator/cli/templates/static/codegen/src/EventProcessing.res(3 hunks)codegenerator/cli/templates/static/codegen/src/UserContext.res(7 hunks)scenarios/test_codegen/src/EventHandlers.ts(2 hunks)scenarios/test_codegen/test/EventHandler_test.ts(1 hunks)scenarios/test_codegen/test/schema_types/BigDecimal_test.res(2 hunks)scenarios/test_codegen/test/schema_types/Timestamp_test.res(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (5)
scenarios/test_codegen/test/schema_types/BigDecimal_test.res (1)
46-48: LGTM – new context flags correctly replaceshouldGroupThe test now passes explicit
shouldSaveHistoryandisPreloadbooleans; this matches the updated context API.scenarios/test_codegen/test/schema_types/Timestamp_test.res (1)
38-40: LGTM – context parameter update is consistentThe switch to
shouldSaveHistory/isPreloadmirrors the loader refactor and compiles cleanly.Also applies to: 48-50
codegenerator/cli/templates/dynamic/codegen/src/Types.res.hbs (1)
29-33: Verify downstream consumers expectisPreloadbefore entity fieldsThe new
isPreloadfield is inserted ahead of the per-entity sub-records.
Double-check that any spread/positional destructuring in generated code (e.g.let {log, effect, user, ...}) follows this order to avoid silent bugs.scenarios/test_codegen/test/EventHandler_test.ts (1)
254-279: Test case looks correctThe additional assertion confirms that
entity.setis ignored during preload runs and guards against regressions.scenarios/test_codegen/src/EventHandlers.ts (1)
457-482: LGTM – solid coverage forisPreloadbehaviourThe new
"loaderSetCount"branch accurately verifies thatsetis ignored on the preload pass and applied on the second run.
Nice defensive assertions oncontext.isPreload.
Co-authored-by: Jono Prest <65739024+JonoPrest@users.noreply.github.com>
| get: (~target as params, ~prop: unknown) => { | ||
| let prop = prop->(Utils.magic: unknown => string) |
There was a problem hiding this comment.
why make prop unknown btw?
There was a problem hiding this comment.
Because it might be either string or symbol
| let set = params.isPreload | ||
| ? noopSet | ||
| : (entity: Internal.entity) => { | ||
| params.inMemoryStore | ||
| ->InMemoryStore.getInMemTable(~entityConfig=params.entityConfig) | ||
| ->InMemoryTable.Entity.set( | ||
| Set(entity)->Types.mkEntityUpdate( | ||
| ~eventIdentifier=params.eventItem->makeEventIdentifier, | ||
| ~entityId=entity.id, | ||
| ), | ||
| ~shouldSaveHistory=params.shouldSaveHistory, |
There was a problem hiding this comment.
Should this be inlined so that it's only constructed if set is accessed?
There was a problem hiding this comment.
I didn't come up with a good way to inline it when I was writing the code. Although, now I have an idea, but I'll keep it like this for now.
There was a problem hiding this comment.
You can just use @inline and make this a function?
But its a nitpick either way.
There was a problem hiding this comment.
@inline doesn't work for function inside of another function. I also tried it first :)
|
Let's better wait till v3 😁 |
|
Who knows - knows 🫡 |
2 participants
Also, hide logs on the first loader run.
I decided not to add
context.oncefor now, since the API is arguable.Summary by CodeRabbit
New Features
Refactor
Tests