refactor(ios): serve media uploads from a handler object, not statics - #631
Draft
jkmassel wants to merge 2 commits into
Draft
refactor(ios): serve media uploads from a handler object, not statics#631jkmassel wants to merge 2 commits into
jkmassel wants to merge 2 commits into
Conversation
The closure form of `start` can't capture the object that owns the server: the closure has to exist before the server does, and retrofitting `self` would form `owner -> HTTPServer -> handler -> owner`, so the owner's deinit — and its `stop()` — would never run. A consumer with dependencies to hold therefore ends up with static functions threading a context parameter through every call, which is how MediaUploadServer is written today. Add an `HTTPRequestHandler` protocol and a `start` overload that takes one. The dependencies become stored properties and the request logic becomes instance methods. The protocol is deliberately not `AnyObject`-constrained: a struct conformer cannot participate in a reference cycle at all, so the ownership question doesn't arise. A final class works too, under the same leaf discipline HTTPServerDelegate already documents. The closure overload is unchanged and forwards to the same code path, so this is purely additive — no existing caller, test, or the debug server is affected. Request handling is mandatory, so it can't be a defaulted HTTPServerDelegate method the way optional customization points are; hence an overload rather than a new delegate requirement.
`MediaUploadServer` handled requests through static functions threading an `UploadContext` parameter through every call, because the closure form of `HTTPServer.start` can't capture the object that owns the server: the closure has to exist before the server does, and capturing `self` would form `MediaUploadServer -> HTTPServer -> handler -> MediaUploadServer`, so `deinit` — and its `stop()` — would never run. The previous commit added `HTTPRequestHandler` for exactly this. The dependencies become stored properties on a `Handler` struct and the request logic becomes instance methods; a value type can't participate in a reference cycle, so the ownership question doesn't arise. Mechanically: `handleRequest` becomes `handle`, the functions that use the dependencies become instance methods, and the ones that don't (`attachmentId`, `relayResponse`, `uploadErrorResponse`, `formFields`) stay static. `UploadContext` goes away — `Handler` is what it was. Helpers outside the handler (`errorResponse`, `writeStream`, `sanitizeFilename`, `uploadsTempDirectory`) are qualified rather than moved. No behavior change: only this file is touched, and no test changed.
jkmassel
added
[Type] Task
6 tasks
XCFramework BuildThis PR's XCFramework is available for testing. Add the following to your .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/631")Built from 5783bdb |
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
2 participants
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.
Stacked on #630. Eighth of ten PRs splitting #621. No behavior change, and no test changed — the review is "did any body change?"
What?
Two commits:
1.
HTTPRequestHandlerinGutenbergKitHTTPAn
HTTPServer.startoverload taking a handler object rather than a closure. Purely additive: the closure overload is unchanged and forwards to the same code path, so no existing caller, test, or the debug server is affected.2.
MediaUploadServerserves from oneThe statics threading an
UploadContextbecome aHandlerstruct with stored properties.Why?
The closure form of
startcan't capture the object that owns the server: the closure has to exist before the server does, and retrofittingselfwould formMediaUploadServer → HTTPServer → handler → MediaUploadServer, so the owner'sdeinit— and itsstop()— would never run. A consumer with dependencies to hold therefore ends up with static functions threading a context parameter through every call, which is howMediaUploadServeris written today.HTTPRequestHandleris deliberately notAnyObject-constrained: astructconformer cannot participate in a reference cycle at all, so the ownership question doesn't arise. Afinal classworks too, under the same leaf disciplineHTTPServerDelegatealready documents.Request handling is mandatory, so it can't be a defaulted
HTTPServerDelegatemethod the way optional customization points are — hence an overload rather than a new delegate requirement.How?
handleRequestbecomeshandle; the functions that use the dependencies become instance methods, and the ones that don't (attachmentId,relayResponse,uploadErrorResponse,formFields) stay static.UploadContextgoes away —Handleris what it was. Helpers outside the handler (errorResponse,writeStream,sanitizeFilename,uploadsTempDirectory) are qualified rather than moved.The restructure commit touches one file and changes no test, which is the point: it is 278/277 lines of churn that would otherwise sit in the middle of an API-design diff.
Testing Instructions
swift test— host suite green, unchangedHTTPServeroverloadxcodebuild