pr05 Typescript #3: Migrate client/utils folder #3553

clairep94 · 2025-07-26T22:21:05Z

pr05 Typescript Migration 3: Migrate the client/utils folder

IMPORTANT: Should be reviewed after #3540 for clarity

This branch is built off of the branch for pr05 Typescript #2: automatically resolve imports of ts files & migrate instance of client/utils file #3540
Relevant commits start at 5b1fd1e

Context:

Rebuild of Pr05/migrate client utils clairep94/p5.js-web-editor#5 --> forgot to do git mv for the first few files and directly renamed, so this looks like deleting and creating a new file for git, and erases the previous history.
Migrate as much of the client/utils folder with the following steps:

git mv someUtil.js someUtil.ts. If possible, commit without the no-verify flag, otherwise with no-verify.
I forgot to add no-verify to my commit message in some of these initial ones
Add unit test to secure current behaviour
Check for all instances of useage for the utility function to determine types & edgecases of types
Add types & handle edgecases & update test
Refactor if beneficial
Add JSDocs

I skipped the intellisense autogen files & the codemirror utility files due to unfamiliarity, but migrated most of the rest of the folder

Changes:

Please see annotations on files

Notes:

I have verified that this pull request:

has no linting errors (npm run lint)
has no test errors (npm run test)
is from a uniquely-named feature branch and is up to date with the develop branch.
is descriptively named and links to an issue number, i.e. Fixes #123

…recated

clairep94 · 2025-07-28T11:38:36Z

client/modules/IDE/components/Header/Nav.jsx

Refactor the portions of using getConfig for readability instead of inline

use new parseBoolean function to parse the string val of getConfig to a boolean instead
I wonder if this file was previously only checking if process.env.LOGIN_ENABLED had a value??

So if the env file looked like:

LOGIN_ENABLED=false UI_COLLECTIONS_ENABLED=false

getConfig('LOGIN_ENABLED') // "false"
!getConfig('LOGIN_ENABLED') // false??

clairep94 · 2025-07-28T11:40:19Z

client/utils/consoleUtils.test.ts

tried my best with this test, I'm not 100% sure how the line offset works, so I didn't test its calculation, just that it should be a number in the tupple that's returned

clairep94 · 2025-07-28T11:43:07Z

client/utils/device.ts

PR > Files changed view on Github shows that this is a delete + new file, due to too many changes from the original due to refactoring, so let me know if it would be preferable to go back to the unrefactored!

I refactored for readability & handling the edgecase of not having a navigator or navigator.userAgent, but that might be excessive

clairep94 · 2025-07-28T11:45:34Z

client/utils/dispatcher.ts

Similar to above, GH shows this in PR mode as a deletion + new file, but it seems the history is still retained when you view git blame:
https://github.com/processing/p5.js-web-editor/blame/2362807016746d3b3798085437c2406db2d31757/client/utils/dispatcher.ts

Kept in the comments from a previous maintainer & added JSDocs. I think the number of new lines of jsdocs made it pass the diff tolerated percentage before github shows it as a file deletion instead of a file update

No changes other than adding types & no refactors on this one

clairep94 · 2025-07-28T11:47:05Z

client/utils/evaluateExpression.test.ts

Not 100% sure if these tests match the function's useage on previewEntry and if there are other cases that need to be added?

clairep94 · 2025-07-28T11:54:11Z

client/utils/formatDate.ts

Small refactor for readability + added JSDocs. History is retained despite looking like a delete + new file here
https://github.com/processing/p5.js-web-editor/blame/2362807016746d3b3798085437c2406db2d31757/client/utils/formatDate.ts

Did small refactor to return '' early if !parsed

Also corrected what I thought was a typo here:
https://github.com/processing/p5.js-web-editor/pull/3553/files#diff-e2130c2674d12d8f2724335d2d8d2b7ade8d366efa4bf42717c2af4eec4fa6aaL28

const diffInMs = differenceInMilliseconds(now, parsed); if (Math.abs(diffInMs < 10000)) { return i18next.t('formatDate.JustNow'); } else if (diffInMs < 20000) { return i18next.t('formatDate.15Seconds'); } else if (diffInMs < 30000) { return i18next.t('formatDate.25Seconds'); } else if (diffInMs < 46000) { return i18next.t('formatDate.35Seconds'); }

It seems like they're using Math.abs incorrectly --> it's using Math.abs(true) // 1 and Math.abs(false) // 0

I updated to be :

const diffInMs = Math.abs(differenceInMilliseconds(now, parsed)); // rest of checks for what range the absolute diff between now and the time something happened falls under

clairep94 · 2025-07-28T11:57:31Z

client/utils/getConfig.ts

History retained here:
https://github.com/processing/p5.js-web-editor/blame/2362807016746d3b3798085437c2406db2d31757/client/utils/getConfig.ts

Refactors:

Remove the circular logic at
https://github.com/processing/p5.js-web-editor/pull/3553/files#diff-8fcb54386d32ff76c5399959faeb4e72bad616564277e8774dff9b20d9fa55b3L1-L4

Made an internal _getConfig function that doesn't warn when the value of the key doesn't exist --> use this to check if the environment is test or not

Update getConfig to also take nullishString as an option property --> if true, return '' instead of undefined when no value exists.

For when we want a number or boolean I added a new utility file called parseStringToType below

I was going to do the parsing within getConfig, as I did with my proof-of-concept PR from my grant application, but I thought actually keeping the parsing in a separate function keeps things more modular and testable & keeps the return types of getConfig consistent for predictability

clairep94 · 2025-07-28T12:00:35Z

client/utils/language-utils.ts

History retained here:
https://github.com/processing/p5.js-web-editor/blame/2362807016746d3b3798085437c2406db2d31757/client/utils/language-utils.ts

Create a unit test to secure all behaviour for this function prior to refactor, then did a refactor to make the long for-loop & nested if-statements more readable

Previous implementation for contxt:

https://github.com/processing/p5.js-web-editor/pull/3553/files#diff-3500dc8e015b1b12abf5952f3e882a00798c9ce666b6eba2554b96735d5398eeL14-L77

clairep94 · 2025-07-28T12:03:32Z

client/utils/metaKey.js

this old implementation uses navigator.platform which is deprecated:
https://developer.mozilla.org/en-US/docs/Web/API/Navigator/platform

I updated this file to use utils/device to determine if the client is using a mac

clairep94 · 2025-07-28T12:05:25Z

client/utils/parseStringToType.ts

New utility functions to parse a string (from getConfig) to a specified type:

for parseNumber it returns a number if parseable or undefined/0 depending on the nullishNumber flag

for parseBoolean it returns a boolean if parseable or undefined/false depending on the nullishBoolean flag

clairep94 · 2025-07-28T12:11:14Z

client/utils/reduxFormUtils.ts

History retained here:
https://github.com/processing/p5.js-web-editor/blame/2362807016746d3b3798085437c2406db2d31757/client/utils/reduxFormUtils.ts

Moved the username regex to top of file for readability

Changed the username length check to .length instead of regex for readability: https://github.com/processing/p5.js-web-editor/pull/3553/files#diff-932633409951987d095611a326d0b6b624bd482b07f1e27a9ca7987f6b52a71bR32

Moved the internal helper functions higher, and grouped the public functions together on the bottom of the file

Added types -- let me know if this organisation makes sense as this was the most stylistic/open-ended thing I did on the PR

Also not sure if we want to have a root/types folder, or if it makes sense to keep types collocated where they are most used, and potentially exported, as I did here?

No other refactors in each function internally

clairep94 · 2025-07-28T12:15:54Z

@khanniie @raclim Ready for review! I tried to annotate the files with comments to provide context / explainations for any refactors, but let me know if we should do a sync-review of this one since it's so big

khanniie

still working through the files but thought I'd sent a first wave!

khanniie · 2025-08-04T04:42:32Z

client/modules/IDE/actions/project.js

@@ -15,7 +15,7 @@ import {
 } from './ide';
 import { clearState, saveState } from '../../../persistState';

-const ROOT_URL = getConfig('API_URL');
+const ROOT_URL = getConfig('API_URL', { nullishString: true });


what do we think the "correct" or expected behavior would be if there was no API_URL? like is "/projects/${projectId}/zip" a valid ROOT_URL or would that cause a different error down the line? or in what cases is it undefined?

khanniie · 2025-08-04T04:52:36Z

client/utils/getConfig.ts

+ * Internal function to retrieve env vars, with no error handling.
+ * @returns String value of env variable or undefined if not found.
+ */
+function _getConfig(key: string): string | undefined {


FYI i know we don't enforce this very consistently but i dont think we are meant to use underscores in naming, as we "follow" the airbnb guide https://github.com/airbnb/javascript?tab=readme-ov-file#naming--leading-underscore

khanniie · 2025-08-10T15:34:58Z

client/modules/IDE/actions/project.js

@@ -307,6 +307,8 @@ export function cloneProject(project) {
      (file, callback) => {
        if (
          file.url &&
+          S3_BUCKET &&
+          S3_BUCKET_URL_BASE &&


good call!!

khanniie · 2025-08-10T15:42:43Z

client/utils/parseStringToType.ts

+  str?: string,
+  nullishNumber = false
+): number | undefined {
+  if (str == null) {


if we're letting string be typed as str?, then is undefined also a valid nullish value? should it be the more general if(!str) ?

khanniie · 2025-08-10T15:43:07Z

client/utils/parseStringToType.ts

+  str?: string,
+  nullishBool = false
+): boolean | undefined {
+  if (str == null) {


khanniie · 2025-08-11T15:46:56Z

client/utils/device.ts

+ *   - see https://developer.mozilla.org/en-US/docs/Web/API/Navigator/platform
+ */
+export function isMac(): boolean {
+  return typeof navigator?.userAgent === 'string'


do we need typeof ? why not something like this:

navigator?.userAgent?.toLowerCase().includes('mac') ?? false

khanniie · 2025-08-11T15:51:55Z

client/utils/dispatcher.ts

+} = {};
+let frameIndex = 1;
+
+/** Codesandbox dispatcher message types */


wonder if we can just use typescript enums here instead?

enum MessageTypes = {
START,
STOP,
etc
}

khanniie · 2025-08-11T15:52:17Z

client/utils/dispatcher.ts

+ */
+export type Message = {
+  type: MessageType,
+  payload?: any


is there any way to use unknown instead of any ?

khanniie · 2025-08-11T15:53:06Z

client/utils/dispatcher.ts

+function eventListener(e: MessageEvent) {
+  const { data } = e;
+
+  // should also store origin of parent? idk


i assume this is leftover from the js file but should we remove the dead code comments?

khanniie · 2025-08-11T15:53:54Z

client/utils/evaluateExpression.ts

+
+type EvalInClosureFn = (expr: string) => EvalResult;
+
+function __makeEvaluateExpression(evalInClosure: EvalInClosureFn) {


avoid using underscores for naming functions

khanniie · 2025-08-11T15:54:56Z

client/utils/evaluateExpression.ts

-function __makeEvaluateExpression(evalInClosure) {
-  return (expr) =>
+type EvalResult = {
+  result: any,


i haven't looked closely enough at where this is used, but is there any way to avoid using any here as well? can we do unknown and then cast it later after doing some type guard checks?

https://www.typescriptlang.org/docs/handbook/advanced-types.html#type-guards-and-differentiating-types

khanniie · 2025-08-11T15:56:13Z

client/utils/evaluateExpression.ts

@@ -19,7 +26,11 @@ function evaluateExpression() {
      }
      result = (0, eval)(newExpr); // eslint-disable-line


can we change this eslint disable to be more specific to what it's disabling?

khanniie · 2025-08-11T15:58:20Z

client/utils/formatDate.ts

+  return null;
+}
+
+export default {


I'm not sure exactly how this is getting imported but i wonder if we can just export these individually instead of in a dict wrapped as one default? totally understand that it's a holdover from the js file though

khanniie

still working through the files but have a few more comments!

khanniie · 2025-08-11T16:07:03Z

client/utils/getConfig.ts

+ */
+function _getConfig(key: string): string | undefined {
+  const env: Record<string, string | undefined> =
+    (typeof global !== 'undefined' ? global : window)?.process?.env || {};


why not do global !== undefined ?

or even

const configSource = global ?? window; const env = configSource?.process?.env ?? {}

khanniie · 2025-08-11T16:10:31Z

client/utils/getConfig.ts

+  return env[key];
+}
+
+type GetConfigOptions = {


should this be an interface? maybe using optional?

https://google.github.io/styleguide/tsguide.html#prefer-optional-over-undefined

khanniie · 2025-08-11T16:11:12Z

client/utils/getConfig.ts

+  return value;
+}
+
+export default getConfig;


same here can we convert to named export? https://google.github.io/styleguide/tsguide.html#exports

khanniie · 2025-08-11T16:12:40Z

client/utils/language-utils.ts

+  supportedLanguages: string[] = [],
+  defaultLanguage: string = 'en'
+): string | undefined {
+  if (typeof navigator === 'undefined') {


same here i think we should be able to do something like if (!navigator) return defaultLanguage

khanniie · 2025-08-11T16:13:49Z

client/utils/language-utils.ts

+  return defaultLanguage;
+}
+
+export default getPreferredLanguage;


can we put the default export on the function itself here too?

or even better... maybe we can make it a named export instead

https://google.github.io/styleguide/tsguide.html#exports

khanniie · 2025-08-11T16:17:16Z

client/utils/reduxFormUtils.ts

@@ -0,0 +1,156 @@
+import i18n from 'i18next';
+
+/* eslint-disable */


what are we disabling here? can we be more specific about the rule?

khanniie · 2025-08-11T16:24:31Z

client/utils/reduxFormUtils.ts

+/** Processes form & mutates errors to add any `username` & `email` errors */
+function validateUsernameEmail(
+  formProps: Partial<UsernameAndEmail>,
+  errors: Partial<FormErrors>


I'm not sure i understand what happens when we do Partial<Partial<....>> do we need the double partial?

khanniie · 2025-08-11T16:25:19Z

client/utils/reduxFormUtils.ts

+
+/** Validation for the Account Form */
+export function validateSettings(
+  formProps: Partial<AccountForm>


if AccountFormErrors is already Partial<AccountForm> then can we also just use it for formProps?

khanniie · 2025-08-11T16:25:59Z

client/utils/reduxFormUtils.ts

+export type LoginFormErrors = Partial<LoginForm>;
+
+/** Validation for the Login Form */
+export function validateLogin(formProps: Partial<LoginForm>): LoginFormErrors {


same here, why not reuse LoginFormErrors? or rename it to be more general?

clairep94 added 30 commits July 26, 2025 16:06

apiClient: update extension to ts

a52d312

apiClient: add axios instance type

2480bd1

device: update extension to ts

87adc72

device: add test

9efdd25

eslint: remove prefer-default-export rule

462524c

device.ts: add edgecase test and refactor

8010af7

metaKey.js: update extention to ts

cb9ddc5

metaKey: update to use isMac() -- due to navigator.platform being dep…

f7282c2

…recated

useKeyDownHandler: refactor to use isMac()

411eb4a

language-utils.js: update ext to ts

e7eaa2c

language-utils.ts: add test

53962db

language-utils: add types, passes typecheck

3efc164

language-utils: refactor for clarity & disable eslint noplusplus rule

fa6d69f

formatDate: update ext to ts

75b35a3

formatDate.js: add test

b7fb7a0

formatDate.js: add types and refactor

2440d6f

consoleUtils: update extension to ts

0a9af47

consoleUtils: add tests and bare minimum type

5302f54

consoleUtils.ts: add jsdocs and return type

2638c32

dispatcher.js: update extention to ts --no-verify

723cac3

dispatcher: add unit test

e0cb6c7

dispatcher.ts: update with types

0aab549

dispatcher.ts: add jsdocs

60f8a6e

remove jsdocs on internal functions to retain git history?

04ca29a

evaluateExpression: update ext to ts --no-verify

e91000f

evaluateExpression: add unit test

0713173

evaluateExpression: add tests

16d84e1

reduxFormUtils: update ext to ts --no-verify

6957124

reduxFormUtils: add unit test, no-verify

97754db

reduxFormUtils: delete unused dom-onlyprops function

ed0c6f0

clairep94 commented Jul 28, 2025

View reviewed changes

clairep94 marked this pull request as ready for review July 28, 2025 12:14

Merge branch 'develop' into pr05/migrate_client_utils_folder

1f5614c

khanniie requested a review from raclim August 1, 2025 16:14

Merge branch 'develop' into pr05/migrate_client_utils_folder

8febdcb

clairep94 mentioned this pull request Aug 3, 2025

pr05 Typescript Migration #5: Migrate client/common folder #3565

Open

14 tasks

khanniie reviewed Aug 10, 2025

View reviewed changes

khanniie reviewed Aug 11, 2025

View reviewed changes


		type EvalInClosureFn = (expr: string) => EvalResult;

		function __makeEvaluateExpression(evalInClosure: EvalInClosureFn) {

		@@ -19,7 +26,11 @@ function evaluateExpression() {
		}
		result = (0, eval)(newExpr); // eslint-disable-line

		@@ -0,0 +1,156 @@
		import i18n from 'i18next';

		/* eslint-disable */

Uh oh!

pr05 Typescript #3: Migrate client/utils folder #3553

Are you sure you want to change the base?

pr05 Typescript #3: Migrate client/utils folder #3553

Uh oh!

Conversation

clairep94 commented Jul 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

pr05 Typescript Migration 3: Migrate the client/utils folder

Context:

Changes:

Notes:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clairep94 Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clairep94 Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clairep94 commented Jul 28, 2025

Uh oh!

khanniie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khanniie Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khanniie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

clairep94 commented Jul 26, 2025 •

edited

Loading

clairep94 Jul 28, 2025 •

edited

Loading

clairep94 Jul 28, 2025 •

edited

Loading

khanniie Aug 11, 2025 •

edited

Loading