Repository navigation
fix: after-middleware re-entry, notification read state, and DotPath/type/build fixes - #11
Merged
Merged
Conversation
After middleware ran on the response's finish event with the real next(), so calling next() (as requestLogger does) resumed the router after the response was sent and raised "Cannot set headers after they are sent to the client". Give it a no-op next and report its errors instead of leaving them unhandled. Add regression tests for the Express and H3 drivers.
…rPage
markAllRead() used the single-row update() and so marked only one
notification; use updateFrom(). unreadForUser(user, perPage) now passes
perPage to paginate(), and both filter unread rows with whereNull('readAt').
DotPath recursed without limit into every object, so a config holding nodemailer transports failed to typecheck with "circularly references itself" wherever config() was called. Bound the depth and stop at built-in object types, and drop the @ts-expect-error workarounds this needed in the H3 template and the notifications configure() helper.
- build:templates ran each template's prebuild in parallel, and each rebuilt the shared packages at once, so builds clobbered one another. - AuthenticationException stored a Request<unknown> in a Request<User> field; two auth tests asserted on loosely typed values. - renderError used the view() global, which common has no type for.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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.
Summary
Bug fixes found while building notifications on Arkstack 0.19 in an app, plus pre-existing typecheck and build failures in the repo.
driver-express:
aftermiddleware re-entered the router (c9a33219)aftermiddleware ran on the response'sfinishevent with the request's realnext(). A middleware that callsnext()(asrequestLogger()does) therefore sent every request back through the remaining routes after the response had been sent. Downstream layers then raisedCannot set headers after they are sent to the client, and a handler could in principle run twice. It now receives a no-opnext, and its errors are logged. Regression tests cover Express and H3; H3 was not affected.notifications: read state (
87025435)UserNotificationCenter.markAllRead()used arkormx's single-rowupdate(), so it marked one notification. It now usesupdateFrom().unreadForUser(user, perPage)never passedperPagetopaginate().whereNull('readAt').common:
DotPathrecursion (f1d68455)DotPath, the key type behindconfig()andconfigure(), recursed without limit into every object. A config holding nodemailer transports therefore failedtscin the Express templates with "circularly references itself" whereverconfig()was called. It is now depth-bounded (7) and stops at built-in object types. This also removes the@ts-expect-errorworkarounds in the H3 template and innotifications/src/config.ts.Pre-existing build and type failures (
48c2a53d)build:templatesran each template'sprebuildin parallel. Each one rebuilt the shared packages at once, so the builds clobbered each other's output and the step failed intermittently. Templates now build one at a time.@arkstack/authfailedtsc:AuthenticationExceptionstored aRequest<unknown>in aRequest<User>field, and two tests asserted on loosely typed values.@arkstack/common'srenderErrorused theview()global without a type for it.Also included
73386b9faddsinlinedDependenciestopackages/common/package.json(maintainer commit that was on localmain).Test plan
pnpm lint,pnpm build,pnpm build:templates,pnpm docs:buildvitest run: 571 passed, 5 skippedtsc --noEmit: 0 errors in theexpress,h3andexpress-inertiatemplates and inauth,http,notifications,realtimeandschedulermarkAllRead/unreadForUserquery shape