refactor: Initial Settings, retry, and fetch refactor - #168
Conversation
🦋 Changeset detectedLatest commit: 82de3ba The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| export { Trustmark } from "./Trustmark.js"; | ||
| export { isActionsAssertion } from "./assertions.js"; | ||
| export * from "./Settings.js"; | ||
| export * from '@contentauth/c2pa-utilities'; |
There was a problem hiding this comment.
Re-exporting from utilities for client convenience.
| vi.mock("node-fetch", () => ({ | ||
| default: vi.fn(), | ||
| })); | ||
| import { loadSettingsFromFile } from "./Settings.js"; |
There was a problem hiding this comment.
Tests are consolidated and moved into the settings.spec.ts in c2pa-utilities. Only the one Node-specific function is tested here.
|
|
||
| export interface BuilderInterface { | ||
| /** An intent lets the API know what kind of manifest to create. | ||
| /** |
There was a problem hiding this comment.
Small formatting adjustment.
| dispose: () => void; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
This comment was leftover in c2pa-web/src/index.ts. Not sure when it got lost, but it's meant to be attached to this function, so moving it over here now.
| READER_SUPPORTED_FORMATS | ||
| } from './lib/supportedFormats.js'; | ||
|
|
||
| export type { |
There was a problem hiding this comment.
These are now re-exported from index.ts.
| @@ -0,0 +1,436 @@ | |||
| /** | |||
There was a problem hiding this comment.
This file unifies Settings-related logic from both Web and Node SDKs, in some cases widening types and function signatures to support both, but without changing behavior. It also re-organizes code into more logical chunks.
| export interface BuilderSettings { | ||
| /** | ||
| * Whether to generate a C2PA archive (instead of zip) when writing the manifest builder. | ||
| * | ||
| * Note: `c2pa-rs` is deprecating the zip archive path — this setting is expected to be | ||
| * removed in a future release and should always be left `true`. | ||
| */ | ||
| generateC2paArchive?: boolean; | ||
| /** | ||
| * Settings for automatic thumbnail generation. | ||
| */ | ||
| thumbnail?: { | ||
| /** | ||
| * Whether to automatically generate a thumbnail for the asset being built, if possible. | ||
| */ | ||
| enabled?: boolean; | ||
| }; | ||
| } |
There was a problem hiding this comment.
This was union'd between the two packages. One had generateC2paArchive as an optional setting, and the other had thumbnail as an optional setting. They now both exist here in the new BuilderSettings as optional.
| export async function resolveSettings( | ||
| baseSettings: Settings | undefined, | ||
| overrideSettings: Settings | undefined, | ||
| options?: FetchWithRetryOptions |
There was a problem hiding this comment.
These options could be generalized going forward, rather than being specific to FetchWithRetryOptions. However, at the moment, the only options that are passed in are specific to fetch-with-retry configuration, so I'm opting to name it as such for now.
| @@ -0,0 +1,164 @@ | |||
| /** | |||
There was a problem hiding this comment.
Considering naming this file just fetch.ts.
tmathern
left a comment
There was a problem hiding this comment.
- Please clarify the deprecation note in the types, it is confusing.
- Also review comments in settings.
cdmurph32
left a comment
There was a problem hiding this comment.
My only concern here is that this doesn't break anything. Could you please add tests in the c2pa-js and c2pa-node repositories to ensure the settings are applied?
tmathern
left a comment
There was a problem hiding this comment.
(Probably also update the PR description, I got no search hit on resolveTrustAnchors in this changeset, other mentioned functions are here).
This initial refactor is centered around the introduction of a new package:
c2pa-utilities, and unifiesSettings, retry, and fetch logic shared betweenc2pa-webandc2pa-node.Across the board, no new functionality was added. Types or functions maintain their current behavior, but are widened in some cases, due to the unification of shapes across Web and Node, in order to maintain support for both. This is not necessarily the final shape; this just helps put everything in one place so that it will be easier to iterate on moving forward.
New additions
The new
c2pa-utilitiespackage contains runtime-agnostic logic thatc2pa-webandc2pa-nodepreviously implemented independently (and had drifted on):Settings/TrustSettings/CawgTrustSettings/VerifySettings/BuilderSettings: now one shared shape, mirroring structs inc2pa-rsresolveSettings,createTrustSettings/createCawgTrustSettings/createVerifySettings,mergeSettings,settingsToJson,snakeCaseify,loadSettingsFromUrlresolveTrustSettingsAdditionally, this package houses generic HTTP retry mechanics that was also previously implemented by both packages. The new
fetchWithRetry.tsmodule implements fetch-with-retry with exponential backoff and handlesRetry-Afterheaders along with configurable options, which will allow the Web and Node packages to inject their own runtime-specific behaviors.Removals
c2pa-weblib/settings.tsremoved entirely; re-exportsc2pa-utilitiesdirectly fromindex.tsfor client conveniencec2pa-nodeSettings.tsreduced to justloadSettingsFromFile(unique to Node thanks to its file system access), plus a re-export of everything else via its respectiveindex.tsfor client convenience.TrustConfig/VerifyConfig/SettingsContextkept as@deprecatedtype aliases for backward compatibility, but otherwise are just re-exposing the unified types now defined inc2pa-utilitiesNotable behavior changes / new capabilities
mergeSettingsnow deep-merges nested fields (e.g.builder.thumbnail) instead of overwriting whole sectionsc2pa-nodegains support for nested arrays for trust-anchor fields (previously only a web feature) and a new opt-inresolveTrustAnchors()for validated fetchingsnakeCaseify: array values were being flattened into{"0": ..., "1": ...}objects instead of preserved as arraysTesting
Retry-Aftervariants, non-Errorrejections, size-cap enforcement, PEM validation acrosssettings.spec.tsand the newfetchWithRetry.spec.tsFollow-up work
Several smaller PRs will be coming up containing their own bite-sized standalone changes, targeting this one as the base branch:
SigningAlgand other shared enums