feat(auth): accept optional features in prefetch()#1643
Conversation
|
|
jmcbgaston
force-pushed
the
feat/prefetch-features-param
branch
from
3a5b1fe to
68e67ed
Compare
| options.features = { | ||
| ...this.options.features, | ||
| migrateAccessTokenToHeader: isAccessTokenHeaderEnabled, | ||
| }; |
There was a problem hiding this comment.
Note on the asymmetry between options.features.migrateAccessTokenToHeader = ... here vs. options.isDocFirstPrefetchEnabled = isDocFirstPrefetchEnabled above:
The param shape matches isDocFirstPrefetchEnabled (top-level boolean coming into prefetch()), but the destination differs because the two flags are read differently by viewers:
isDocFirstPrefetchEnabled→ read directly offthis.options:const { isDocFirstPrefetchEnabled } = this.options(DocBaseViewer.js:464)migrateAccessTokenToHeader→ read viathis.featureEnabled('migrateAccessTokenToHeader'), which pulls fromthis.options.features. Used in ~14 call sites acrossDocBaseViewer,ImageViewer,MultiImageViewer,ArchiveViewer,PlainTextViewer,CSVViewer,BaseViewer.
So the param matches the existing top-level shape, but the destination has to land in options.features to align with how viewers already consume it (and with how PreviewContent.tsx sets it on the show() path). Placing it top-level instead would require refactoring every viewer call site.
| sharedLinkPassword, | ||
| isDocFirstPrefetchEnabled: false, | ||
| docFirstPagesConfig: null, | ||
| features: { migrateAccessTokenToHeader: false }, |
There was a problem hiding this comment.
should we update this to isAccessTokenHeaderEnabled also?
| options.docFirstPagesConfig = docFirstPagesConfig; | ||
| options.features = { | ||
| ...this.options.features, | ||
| migrateAccessTokenToHeader: isAccessTokenHeaderEnabled, |
prefetch() relied on this.options.features which is only set by show(). Callers that prefetch before show() (e.g. folder hover prefetch) had no way to pass features into the viewer, so featureEnabled() checks in viewer prefetch() methods always read defaults. Add an optional features param to prefetch() and merge it into per-call viewer options without mutating this.options.
prefetch() relied on this.options.features which is only set by show(). Callers that prefetch before show() (e.g. folder hover prefetch) had no way to opt into auth-header routing, so featureEnabled() checks in viewer prefetch() methods always read defaults. Add an optional isAccessTokenHeaderEnabled top-level param to prefetch() (matching the isDocFirstPrefetchEnabled shape). When set inside the preload branch, the viewer's options.features.migrateAccessTokenToHeader gets the threaded value without mutating this.options.
jmcbgaston
force-pushed
the
feat/prefetch-features-param
branch
from
9793a36 to
cb43514
Compare
|
Reopening from upstream branch so Mergify's strict-merge queue can operate (the queue check failed because this PR was from a fork). Replacement PR: #1645 |
4 participants
Description
prefetch()relied onthis.options.features, which is only set duringPreview.show(). Callers that prefetch beforeshow()(for example, folder-list hover prefetch in a host application) had no way to opt into auth-header routing for the access token, sofeatureEnabled('migrateAccessTokenToHeader')checks inside viewerprefetch()methods always read the default (false).This change adds an optional top-level
isAccessTokenHeaderEnabled = falseparam toprefetch(), matching the shape of the existingisDocFirstPrefetchEnabledparam. When the prefetch is forpreload, the per-call viewer options merge infeatures.migrateAccessTokenToHeaderso viewerprefetch()methods see the threaded value — without mutatingthis.options.features.show()/prefetch()calls aren't affected by prior per-call state.Test plan
yarn test src/lib/__tests__/Preview-test.js— 271 passing, including new tests covering flag-on (feature surfaced to viewer options) and flag-off (feature surfaced asfalse) pathsAuthorization: Bearerheader (noaccess_tokenin URL) when passingisAccessTokenHeaderEnabled: truefalsepreserves existing behaviorSemantic release type
feat- New feature