entity icon indicating copy to clipboard operation
entity copied to clipboard

feat: require explicit query context specification

Open wschurman opened this issue 11 months ago • 2 comments

Why

This is a RFC.

The proposal is to require the queryContext argument to reduce the likelihood of accidental omittance within a transaction block in application code.

At Expo, we've noticed sometimes a transaction will be started and the loader/mutator calls within it may accidentally not include the query context (which must be explicitly supplied as an arg to run within the transaction):

await viewerContext.runInTransactionAsync(async (queryContext) => {
  const existingNote = await NoteEntity.loader(viewerContext) // note how queryContext was accidentally omitted but tsc didn't have an error
    .enforcing()
    .loadByIDAsync(args.id);
  return await NoteEntity.updater(existingNote, queryContext)
    .setField('title', args.note.title)
    .setField('body', args.note.body)
    .enforceUpdateAsync();
});

Closes ENG-11824.

How

The proposed change is to always require queryContext. At Expo, we added a method to our ViewerContext subclass to vend the default EntityQueryContext (non-transactional) for our default database adaptor flavor. I added a similar method to TestViewerContext within this repo to demonstrate the convenience accessor.

RFC

  • Does this pattern make sense?
  • Is the default query context convenience pattern appropriate?

Test Plan

yarn tsc

wschurman avatar Apr 02 '24 23:04 wschurman