Skip to content

refactor: convert CoursewareContainer to TypeScript - #2021

Open
brian-smith-tcril wants to merge 1 commit into
bsmith/react-query-declass-courseware-containerfrom
bsmith/react-query-courseware-container-typescript
Open

refactor: convert CoursewareContainer to TypeScript#2021
brian-smith-tcril wants to merge 1 commit into
bsmith/react-query-declass-courseware-containerfrom
bsmith/react-query-courseware-container-typescript

Conversation

@brian-smith-tcril

@brian-smith-tcril brian-smith-tcril commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Convert CoursewareContainer from .jsx to .tsx. The diff is tiny — a file rename plus a handful of annotations, no behavior change — and that's the point: the value isn't in this PR, it's in what it unlocks. With the container in TypeScript, the courseware React Query peels (#2010+) convert the data layer in TS-land, so every useQuery/useMutation and the code consuming it is type-checked as it lands — instead of writing the conversions in untyped .jsx and retrofitting types later.

Fast-follow to the de-class (#2020); part of the Redux → React Query migration (#1946, Stage 1), stacked above #2020 and below the RQ peels. Split from the de-class so each PR reviews as one idea (structural change vs. type annotations). Closes #2019.

What changed

Only src/courseware/CoursewareContainer.jsx.tsx (git tracks it as a rename; the diff is just the annotations):

  • state: RootState on the inline useSelectors (matching the TabPage.tsx precedent); useRef<any>() on the two loose refs. The redirect helpers and createSelector selectors needed no annotation — reselect infers them.
  • Two small interim bridges to TabPage's already-typed props: courseStatus as CourseStatus at the read, and courseId ?? undefined at the prop. Both fall away when the peels move this component to RQ (it'll pass courseStatus={{ metadataQuery, tabDataQuery }} and take courseId from useParams, like the five course-home tabs already do).

The decision log covers the full rationale — the two bridges, the alternatives weighed (incl. changing the slice), and why store.ts is left untouched.

Testing

npm run types (0 errors), npm run lint (clean), and CoursewareContainer.test.jsx (70 passing) all green — the test file stays .jsx and unchanged, resolving to the .tsx. No behavior change to exercise; a manual click-through of the courseware player confirmed it renders as before.

Decisions

Full decision log

Decisions — convert CoursewareContainer to TypeScript (#2019)

Working notes for this PR (part of the wider Redux → React Query migration,
#1946). Not checked in — referenced when opening the PR. Fast-follow to the
de-class #2008/#2020, stacked directly above it and below the courseware RQ peels
(#2010+). Closes #2019.

This layer is a pure TypeScript conversion: rename
src/courseware/CoursewareContainer.jsx.tsx and add the minimum annotations
to compile. No behavior change. It exists so the courseware RQ peels all happen
in TS-land; it's split from the de-class (#2020) so each PR reviews as one idea
(structural change vs. type annotations).

Don't type the doomed Redux stuff (any/loose)

Decision. The Redux-sourced values are about to be deleted/replaced as each
fetch becomes a typed useQuery in the RQ peels, so type them loosely now and
tighten per-peel. In practice this needed very little:

  • The six exported redirect helpers (checkResumeRedirect, …) and the
    createSelector selectors (currentCourseSelector, …) needed no
    annotation — reselect's defaultMemoize / createSelector types infer their
    parameters, so tsc was already satisfied. No any added there.
  • The two internal refs (latest, guards) hold doomed shapes (the mirrored
    props and the memoized guard fns), so they're useRef<any>().

Dispatch stays plain useDispatch() — no typed-dispatch primitive

Decision. Do not add an AppDispatch type / useAppDispatch hook (store.ts
is untouched). Typed thunk dispatch only ever serves the Redux layer we're deleting:
the RQ-converted code uses useQuery/useMutation and doesn't dispatch at all, and
as each peel converts a fetch its dispatch(...) calls disappear — so there's no
future where typed dispatch helps the RQ code. Adding it would be investing type
effort in the doomed Redux stuff, against the migration's whole point.

It's also unnecessary to compile: the thunks live in thunks.js, so tsc sees them
loosely and plain useDispatch() type-checks the dispatch(fetchCourse(id)) calls
without complaint. (The #2019 issue's task list included adding the primitive — that
premise doesn't hold, so it's dropped; store.ts keeps only its existing RootState.)

useSelector state typed as RootState

Decision. The five inline useSelector((state: RootState) => state.courseware.…)
calls are typed with RootState (matching the TabPage.tsx precedent), which is
enough for state.courseware.* to resolve. The courseware slice is still JS, so
those values come back loosely inferred — fine for now.

Two interim bridges to TabPage's (RQ-era) props

TabPage's prop types are designed for the converted world (CourseStatus = StatusValue | { metadataQuery, tabDataQuery }, courseId?: string), while this
component is still a Redux caller. Typing the state surfaced two gaps — handled
differently, because they are different kinds of gap:

  • courseStatus — assert once at the read. The slice value genuinely is a
    valid StatusValue, but because slice.js is JS, RootState infers the field as
    the wider string, which won't land in CourseStatus on its own. So it's typed
    as CourseStatus at the useSelector (import type from ../tab-page/TabPage),
    making it a valid CourseStatus from its source — not at the prop. The value is
    what the type says; the assertion just tells TS what the JS slice can't.
  • courseId — coerce at the TabPage prop only. courseId={courseId ?? undefined}.
    The slice uses string | null (null = no course yet); TabPage uses string | undefined.
    This one can't move to the read: the effect's ids-mismatch bail
    (courseId !== (routeCourseId || null)) depends on courseId staying string | null
    — coercing it to undefined globally would make undefined !== null fire the bail
    on every unloaded render (a behavior change). So courseId stays string | null
    everywhere and is coerced (a real null→undefined value transform, not a type
    assertion) only for the one consumer that wants undefined. Behavior-safe:
    TabPage treats a falsy courseId the same either way.

Both are interim. When the courseware peels convert this component's data, it'll
pass courseStatus={{ metadataQuery, tabDataQuery }} (the object arm of
CourseStatus, no cast — exactly as the five course-home tabs already do) and take
courseId from useParams/useContextId (string | undefined, matching directly),
and both bridges drop.

Alternatives considered for the courseId null→undefined bridge

We deliberately kept the inline courseId ?? undefined at the TabPage prop after
weighing the alternatives:

  • Make the slice return undefined instead of null. Tempting as "prepping the
    RQ shape," but rejected: the RQ-era courseId does not come from this slice —
    the Tear down the courseware Redux slice + replace useContextId #1976 teardown deletes state.courseware.courseId and sources it from
    useContextId/useParams (already string | undefined). So changing the slice
    makes a soon-deleted field briefly mimic the RQ shape without building toward it —
    i.e. investing in Redux we're removing. It would also touch the behavior-critical
    bail (below), require editing sequenceId for consistency, and need a repo-wide
    audit of ~14 readers of state.courseware.courseId/.sequenceId. Not worth it to
    save one coercion; out of scope for a TS-only PR.
  • Type the whole slice (slice.js.ts). Would remove the courseStatus
    assertion too, but same objection — typing effort spent on the slice that's deleted
    in Tear down the courseware Redux slice + replace useContextId #1976, and out of scope here.
  • Coerce courseId at the read (globally), like courseStatus. Rejected: the
    effect's bail normalizes both sides to null-when-absent —
    courseId !== (routeCourseId || null) — so it relies on courseId being
    string | null. Coercing it to string | undefined makes the unloaded case
    undefined !== nulltrue, firing the bail when it shouldn't (a behavior change).
    Unlike courseStatus (a type assertion of an already-valid value), this is a value
    transform
    that's only correct for the TabPage consumer, so it must stay local to
    that consumer.
  • Named intermediate (const tabPageCourseId = courseId ?? undefined). Equivalent
    to the inline form; chose inline since there's a single consumer and it's one line.
    The "why" lives here in the doc rather than as a code comment.

Tests unchanged

Decision. CoursewareContainer.test.jsx stays .jsx and unchanged — it
imports ./CoursewareContainer, which now resolves to the .tsx, and the 70 cases
(incl. the two coverage tests added in #2020) pass as-is. No behavior change, so no
test edits. npm run types and npm run lint are clean.

Manual testing

Manual testing — convert CoursewareContainer to TypeScript (#2019)

A type-only conversion (.jsx.tsx) with no runtime/behavior change, so
there's nothing new to exercise beyond confirming the courseware player still
renders. The real safety net is tooling: npm run types (0 errors),
npm run lint (clean), and CoursewareContainer.test.jsx (70 passing, unchanged).

Mark results as you go — [x] pass, [!] problem (add a note).

  • Clicked around the courseware player (load, unit navigation) — renders and
    behaves as before. No functional change expected, none observed.

🤖 Generated with Claude Code

@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.53%. Comparing base (c346ad2) to head (2459906).

Additional details and impacted files
@@                                 Coverage Diff                                 @@
##           bsmith/react-query-declass-courseware-container    #2021      +/-   ##
===================================================================================
+ Coverage                                            93.51%   93.53%   +0.01%     
===================================================================================
  Files                                                  363      363              
  Lines                                                 5894     5905      +11     
  Branches                                              1365     1403      +38     
===================================================================================
+ Hits                                                  5512     5523      +11     
+ Misses                                                 367      366       -1     
- Partials                                                15       16       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@brian-smith-tcril
brian-smith-tcril marked this pull request as ready for review August 20, 2026 18:29
@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-courseware-container-typescript branch from 28d3943 to 5422cfe Compare August 21, 2026 00:58
@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-courseware-container-typescript branch from 5422cfe to fe35054 Compare August 21, 2026 01:59
Rename src/courseware/CoursewareContainer.jsx → .tsx with the minimum annotations
to compile. No behavior change; fast-follow to the de-class (#2008/#2020) so the
courseware React Query peels happen in TS-land.

- state: RootState on the inline useSelectors
- useRef<any> for the two loose refs (mirrored props + memoized guards); reselect
  infers the redirect helpers + createSelector selectors, so no annotations there
- courseStatus asserted `as CourseStatus` at the read (a valid CourseStatus from its
  source); courseId coerced `?? undefined` only at the TabPage prop, since the
  effect's ids-mismatch bail needs courseId to stay string | null
- dispatch stays plain useDispatch(); no typed-dispatch primitive added — typed
  thunk dispatch would only serve the Redux being removed

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-courseware-container-typescript branch from fe35054 to 2459906 Compare August 21, 2026 14:48

@arbrandes arbrandes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-approved, with an optional nit.

const sectionViaSequenceId = useSelector(sectionViaSequenceIdSelector);

const latest = useRef();
const latest = useRef<any>();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: latest could carry its real shape instead of any. But I figure this might be going away, so totally optional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Peel: convert CoursewareContainer to TypeScript (fast-follow to de-class #2008)

2 participants