Refactor stale feature banner to be reusable #5574
Conversation
There was a problem hiding this comment.
This is a fantastic refactoring! Moving the complex stale-banner logic out of the component and into a dedicated, reusable utility in utils.ts is a great architectural improvement. The new functions are well-structured, and the comprehensive test suite in utils_test.ts is excellent.
I saw your discussion about using an interface for shippingDateInfo versus a class or a plain object. You absolutely made the right call. Using an interface is the idiomatic best practice here. It provides strong type-safety and documents the data shape without the overhead of a class instance, which is ideal for handling plain data from a JSON API.
I have a few suggestions, mostly focused on tightening up the TypeScript typings to make this new module even more robust and maintainable. Overall, this is a really strong contribution.
| * Returns a warning banner TemplateResult if a feature is considered outdated, | ||
| * otherwise returns null. | ||
| * @param {Feature} feature The feature object. | ||
| * @param {closestShippingDateInfo} shippingInfo The shipping information from findClosestShippingDate. |
There was a problem hiding this comment.
Suggestion: This is a .ts file, so we should use TypeScript's explicit type annotations for function parameters instead of relying on JSDoc. Could we add the : Feature type to the feature parameter here? This would improve type safety and editor support. This comment applies to all the new functions added in this PR (isShippedFeatureOutdatedForAuthor, isShippedFeatureOutdatedForAll, etc.), which are also missing explicit types for their feature and other parameters.
There was a problem hiding this comment.
I've updated this to now use the types effectively. This did, however, chain into a lot of other changes, since the tests required more thorough values to make sure the variables were properly typed. Then, other components that previously used some of these existing functions also needed updating. This even uncovered that there were some data type discrepancies in the Feature type definition, so there's a lot of updates coming with this change. 🙂
client-src/elements/utils.ts
Outdated
| * @returns {boolean} True if the banner should be shown for all users. | ||
| */ | ||
| function isShippedFeatureOutdatedForAll( | ||
| feature, |
There was a problem hiding this comment.
Same thing. Add the types.
client-src/elements/utils.ts
Outdated
| * @returns {boolean} True if the banner should be shown for an author. | ||
| */ | ||
| function isShippedFeatureOutdatedForAuthor( | ||
| feature, |
There was a problem hiding this comment.
Same thing around adding the types instead of relying on jsdocs
client-src/elements/utils.ts
Outdated
| * and booleans for isUpcoming and hasShipped. | ||
| */ | ||
| export async function findClosestShippingDate( | ||
| channels, |
There was a problem hiding this comment.
We should import the Channels and StageDict from cs-client.js and use those
client-src/elements/utils.ts
Outdated
| * @returns {boolean} True if the upcoming feature is outdated, false otherwise. | ||
| */ | ||
| function isUpcomingFeatureOutdated( | ||
| feature, |
There was a problem hiding this comment.
Same thing for adding the types
client-src/elements/utils.ts
Outdated
| } | ||
|
|
||
| // Represent two months grace period. | ||
| const nineWeekPeriod = 9 * 7 * 24 * 60 * 60 * 1000; |
There was a problem hiding this comment.
Suggestion: This is a great use of a fixed week count to approximate a "two-month" period. To make the code a little more self-documenting, what do you think about extracting the magic number into a named constant? We could also clarify the comment slightly.
For example, we could define this at the top of the file:
// A 9-week grace period used to approximate 2 months for shipped features.
const SHIPPED_FEATURE_OUTDATED_GRACE_PERIOD = 9 * 7 * 24 * 60 * 60 * 1000;And then use the constant here. This would make the purpose of the value immediately clear at the point of use.
client-src/elements/utils_test.ts
Outdated
| assert.isNull(result); | ||
| }); | ||
|
|
||
| it('should return a banner if accurate_as_of is missing for upcoming', () => { |
There was a problem hiding this comment.
Suggestion: This test case name, 'should return a banner if accurate_as_of is missing for upcoming', is a little ambiguous. It correctly tests that a banner is shown, but it's not immediately obvious why. What do you think about renaming it to something like 'should return upcoming banner if accurate_as_of is missing' and adding a brief comment inside the test explaining that a missing accurate_as_of date is considered outdated for an upcoming feature? It would make the intent clearer for future maintainers.
There was a problem hiding this comment.
LGTM overall. Some nits around adding more types instead of relying on js docs (JS Docs are great for .js files). I know this was a Refactor with existing stuff. but since we are moving things around, those nits could help out in general.
Also, apologies for the Gemini review. It placed comments in the wrong location. I went back and added what I thought was relevant and resolved other ones.
Part of #5496
(This ended up being more work than I realized)
This change updates the existing logic for displaying the outdated feature banner and moves it to a utility function so that it can be used on any page.