docs: Add TypeScript typings to LmsApiService group methods #1426

marlonkeating · 2025-02-13T21:20:06Z

This change converts LmsApiService into a .ts file, and adds TypeScript typings to methods based around Enterprise Groups. This change is intended as a first step towards annotating all of our api contracts with TypeScript.

For all changes

Ensure adequate tests are in place (or reviewed existing tests cover changes)

Only if submitting a visual change

Ensure to attach screenshots
Ensure to have UX team confirm screenshots

codecov · 2025-02-13T21:28:06Z

Codecov Report

Attention: Patch coverage is 44.44444% with 10 lines in your changes missing coverage. Please review.

Project coverage is 86.43%. Comparing base (02cc629) to head (766a268).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/data/services/LmsApiService.ts	41.17%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1426      +/-   ##
==========================================
- Coverage   86.48%   86.43%   -0.06%     
==========================================
  Files         660      660              
  Lines       14936    14931       -5     
  Branches     3164     3162       -2     
==========================================
- Hits        12917    12905      -12     
- Misses       1952     1956       +4     
- Partials       67       70       +3

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

src/types.d.ts

adamstankiewicz · 2025-02-14T19:57:37Z

src/types.d.ts

@@ -0,0 +1,33 @@
+export type HttpResponse<Payload = {}> = {


[suggestion] It might be worth using AxiosResponse from the transient axios dependency via @edx/frontend-platform, e.g.:

import type { AxiosResponse } from 'axios'; export type EnterpriseGroupResponse = Promise<AxiosResponse<EnterpriseGroup>>;

Nice, I knew this had to be defined somewhere! Thanks for pointing it out.

adamstankiewicz · 2025-02-14T20:00:05Z

src/types.d.ts

+
+export type EnterpriseGroup = {
+  /* uuid of enterprise customer */
+  enterprise_customer: string,


[curious/consideration] Given we typically camelCase API response data properties in service functions, e.g.:

const response = await getAuthenticatedHttpClient().get(url); return camelCaseObject(response.data);

Should the interface represent the API response (snake_case), or how we typically consume it within hooks/components/etc. (camelCase)? For instance, when the interface is defined with snake_case, there's a greater likelihood it could discourage the pattern of camelCasing API data to make working with the data in the frontend feel more like JavaScript (camelCase is the norm in JavaScript/TypeScript).

Worth noting that looking at the resulting types within getEnterpriseGroup on your current branch, the types are lost after the camelCaseObject (i.e., const enterpriseGroup = camelCaseObject(response.data); resolves to type any).

Would it make sense to move the camelCaseObject into the service function, and define the type with camelCase properties?

[food for thought / aside] FWIW, while I don't think it's being used, frontend-platform technically supports MFEs using custom middleware with axios (ADR), e.g. integrating the http client with axios-case-converter(docs) within the MFE's initialize call:

initialize({ messages: [appMessages], requireAuthenticatedUser: true, hydrateAuthenticatedUser: true, authMiddleware: [axiosCaseConverter], });

See frontend-platform's implementation where the middleware function (axiosCaseConverter in example above) is called for the client here.

Maybe this might be an interesting topic to discuss in an upcoming Enterprise Solution Review? E.g., if we want to adopt the axios-case-converter auth middleware to be consistent about camelCasing API responses within our Enterprise MFEs 🤔

Overall I would agree that camelCase/snake_case conversions ideally happen in the api libraries, and so I've made that change to the group methods and adjusted their typings accordingly.

In regards to adopting an axios plugin that automatically converts snake_case to camelCase values, I would agree it's a more elegant solution, but don't know if there's a graceful way to integrate it into our code base at this point. The only way I can think to do it without modifying every point in the code that does camelCase conversion, is to carry around two axios client objects during the transition period: where one client does the automatic conversion and the legacy client does not.

Agreed, adopting it gradually would be ideal, though not the worst to tackle all existing usages in one go (N = ~65 usages). Especially given doing so would probably be less overall effort than carrying around two axios clients.

That said, if the axios plugin was enabled without any changes, the camelCaseObject usages we have today would largely be camelCasing data that's already been camelCased, aka largely a no-op operation that doesn't necessarily break anything either (where usages could be removed incrementally, even while the axios plugin is enabled). The primary concern would be if we ever access snake_case properties prior to passing the response data to camelCaseObject, but I don't believe we do on a cursory search through usages.

adamstankiewicz · 2025-02-14T20:04:02Z

src/data/services/LmsApiService.ts

@@ -3,6 +3,22 @@ import { camelCaseObject } from '@edx/frontend-platform/utils';

 import { configuration } from '../../config';
 import generateFormattedStatusUrl from './apiServiceUtils';
+import { EnterpriseGroup, HttpResponse, Paginated } from '../../types';
+
+export type CreateEnterpriseGroupArgs = {


[curious] I'm interested in your thoughts on when to use type vs. interface for object type definitions.

IMO, types are for data specifications, while interfaces are for class/function contracts. In the case of a TypeScript project that is a web app front-end (as opposed to a library or back-end api), I don't see what I consider the ideal use cases for interfaces coming up all that often.

Also, I find the operations available to types more generally useful, especially given the heavily composition-oriented (rather than inheritance-oriented/OOP) paradigm coming from our Javascript codebase.

adamstankiewicz · 2025-02-18T20:40:28Z

tsconfig.json

@@ -1,26 +1,8 @@
 {
+  "extends": "@edx/typescript-config",


adamstankiewicz · 2025-02-18T20:40:52Z

src/types.d.ts

+  next: string?,
+  previous: string?,
+  count: number,
+  current_page: number,


nit: any reason not to camelCase the entire paginated response (i.e., currentPage) vs. only the results list?

None that I could see! Edited.

adamstankiewicz · 2025-02-18T20:46:00Z

src/types.d.ts

+  next: string?,
+  previous: string?,
+  count: number,
+  current_page: number,


[consideration] fwiw, not all paginated responses will have current_page or start in their response. For instance, if an API endpoint doesn't utilize DefaultPagination from edx-drf-extensions.

Some APIs use the base PageNumberPagination from DRF (source):

{ "count": 1023, "next": "https://api.example.org/accounts/?page=5", "previous": "https://api.example.org/accounts/?page=3", "results": [ … ] }

[suggestion] Is it worth creating both a base paginated response and a paginated response conforming to DefaultPagination (with current_page and start)?

Paginated (base)

PaginatedCurrentPage (or similar)

Good call-out! I was not aware.

adamstankiewicz · 2025-02-18T20:51:19Z

package.json

@@ -38,6 +38,7 @@
    "@edx/frontend-enterprise-utils": "9.1.0",
    "@edx/frontend-platform": "8.1.5",
    "@edx/openedx-atlas": "^0.6.0",
+    "@edx/typescript-config": "1.1.0",


Should @edx/typescript-config be in devDependencies, since TypeScript things happen during build/dev, not at runtime (e.g., in learner portal)?

Good catch, I meant to put it there!

adamstankiewicz · 2025-02-18T20:54:04Z

src/data/services/LmsApiService.ts


 import { configuration } from '../../config';
 import generateFormattedStatusUrl from './apiServiceUtils';
+import { EnterpriseGroup, Paginated } from '../../types';


[nit/curious] When is it best to use the Types.* namespace vs. explicitly including imports for the types?

Types.EnterpriseGroup Types.Paginated

I started out using Types. instead of importing, but then saw that some of the deeply nested template types were losing readability. For instance:
export type EnterpriseGroupListResponse = Promise<Types.AxiosResponse<Types.Paginated<Types.EnterpriseGroup>>>;

adamstankiewicz · 2025-02-18T21:17:17Z

src/types.d.ts

+
+export type EnterpriseGroup = {
+  /* uuid of enterprise customer */
+  enterprise_customer: string,


Agreed, adopting it gradually would be ideal, though not the worst to tackle all existing usages in one go (N = ~65 usages). Especially given doing so would probably be less overall effort than carrying around two axios clients.

That said, if the axios plugin was enabled without any changes, the camelCaseObject usages we have today would largely be camelCasing data that's already been camelCased, aka largely a no-op operation that doesn't necessarily break anything either (where usages could be removed incrementally, even while the axios plugin is enabled). The primary concern would be if we ever access snake_case properties prior to passing the response data to camelCaseObject, but I don't believe we do on a cursory search through usages.

tsconfig.json

docs: use AxiosResponse type refactor: LmsApiService group methods convert response to camelCase build: use @edx/typescript-config compilerOptions build: move typescript-config to devDependencies docs: separate pagination types and camelCase fields build: add trailing comma to tsconfig

adamstankiewicz reviewed Feb 14, 2025

View reviewed changes

marlonkeating force-pushed the mkeating/ENT-9742 branch 3 times, most recently from 5c960a5 to 3c9a427 Compare February 18, 2025 19:53

adamstankiewicz reviewed Feb 18, 2025

View reviewed changes

marlonkeating force-pushed the mkeating/ENT-9742 branch from 3c9a427 to d6d0d3a Compare February 19, 2025 00:27

adamstankiewicz approved these changes Feb 19, 2025

View reviewed changes

tsconfig.json Show resolved Hide resolved

marlonkeating force-pushed the mkeating/ENT-9742 branch from d6d0d3a to e610d9b Compare February 19, 2025 18:21

Merge branch 'master' into mkeating/ENT-9742

766a268

marlonkeating merged commit d5fef09 into master Feb 19, 2025
4 of 6 checks passed

marlonkeating deleted the mkeating/ENT-9742 branch February 19, 2025 19:00

This was referenced Feb 19, 2025

revert: docs: Add TypeScript typings to LmsApiService group methods #1432

Closed

fix: createEnterpriseGroup response camelCase #1433

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: Add TypeScript typings to LmsApiService group methods #1426

docs: Add TypeScript typings to LmsApiService group methods #1426

marlonkeating commented Feb 13, 2025 •

edited

Loading

codecov bot commented Feb 13, 2025 •

edited

Loading

adamstankiewicz Feb 14, 2025

marlonkeating Feb 14, 2025

adamstankiewicz Feb 14, 2025

adamstankiewicz Feb 14, 2025 •

edited

Loading

marlonkeating Feb 18, 2025

adamstankiewicz Feb 18, 2025

adamstankiewicz Feb 14, 2025

marlonkeating Feb 14, 2025

adamstankiewicz Feb 18, 2025

adamstankiewicz Feb 18, 2025

marlonkeating Feb 19, 2025

adamstankiewicz Feb 18, 2025

marlonkeating Feb 19, 2025

adamstankiewicz Feb 18, 2025

marlonkeating Feb 18, 2025

adamstankiewicz Feb 18, 2025

marlonkeating Feb 18, 2025

adamstankiewicz Feb 18, 2025

docs: Add TypeScript typings to LmsApiService group methods #1426

docs: Add TypeScript typings to LmsApiService group methods #1426

Conversation

marlonkeating commented Feb 13, 2025 • edited Loading

For all changes

Only if submitting a visual change

codecov bot commented Feb 13, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamstankiewicz Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marlonkeating commented Feb 13, 2025 •

edited

Loading

codecov bot commented Feb 13, 2025 •

edited

Loading

adamstankiewicz Feb 14, 2025 •

edited

Loading