feat(*): typescript prettier vitest #57

code-forger · 2024-11-13T00:44:05Z

Convert this codebase to typescript (from js), prettier (from nothing) and vitest (from jest)

code-forger · 2024-11-13T00:45:13Z

README.md

@@ -43,7 +43,7 @@ reducer.buildInitialState = buildInitialState;
 ```


I will add the new documentation for how to use this library with typescript before this PR can be merged

code-forger · 2024-11-13T00:46:22Z

__tests__/fixtures.ts

+  });
+}
+
+export function runVitruviusImmutableTests(


Its a shame that this function is bifurcated for the sake of the two different type of Vitruvius. However the gymnastics that would be required to do this in a single function like it was before are not worth it for a test file.

aww, by TS overrides are sooooo fun 🤡

code-forger · 2024-11-13T00:47:25Z

__tests__/fixtures.js

-        other: otherReducer,
-        static: staticReducer,
-      });
-      const store = configureStore({ reducer }, reducer.buildInitialState(locals));


Note, this test did not make it into the other file because TS does not allow it. This is because configureStore expects 1 parameter and we are passing 2. I can not find any reference online to this function ever taking 2 parameters.

code-forger · 2024-11-13T00:51:18Z

src/immutable.ts

+import collectBuiltState, { VitruviusImmutableReducers } from './collectBuiltState';
+
+// internal type for use before assigning buildInitial state
+type VitruviusImmutableOptionalCombinedReducer<Locals> = ReturnType<typeof combineReducers> & {


Ironically, the use of ReturnType<typeof combineReducers> actually weakens the types of vitruvius. This is because redux-immutable does not preserve the types of the combined reducers. Therefore when using the immutable variation of vitruvius, callers will not be able to rely on typescript when accessing the resultant store.

I looked briefly into preserving the types of the reducers within vitruvius itself, but it is extremely complex. I recommend we proceed with the above types, and re-visit this hole in the type chain later.

smackfu

Need to circle back to review the code more.

smackfu · 2024-11-13T14:16:24Z

vitest.config.js

@@ -0,0 +1,14 @@
+import { defineConfig } from 'vitest/config';


Maybe make the vitest.config.js a typescript file?

addressed in 5990a67

smackfu · 2024-11-13T14:18:09Z

package.json

@@ -5,27 +5,18 @@
  "main": "lib/index.js",


Think we need to add a types key.

addressed in 5990a67

smackfu · 2024-11-13T14:18:50Z

package.json

    "test:lockfile": "lockfile-lint -p package-lock.json -t npm -a npm -o https: -c -i",
-    "test:unit": "jest",
+    "test:unit": "npm run build --ignore-scripts && vitest --run",


Can you make the build a "pretest:unit" script?

addressed in 5990a67

smackfu · 2024-11-13T14:19:57Z

package.json

    "redux": "^5.0.1",
    "redux-immutable": "^4.0.0",
-    "rimraf": "^5.0.5"
+    "rimraf": "^5.0.5",
+    "typescript": "^5.4.4",


Typescript doesn't follow semver because every change is breaking.

Suggested change

"typescript": "^5.4.4",

"typescript": ">=5.4.4",

wouldn't it be better to pin it then?

Perhaps ~? Or are even patch versions breaking?

Yeah since it's a dev dep it honestly doesn't really matter.

I just like pointing it out.

addressed in 5990a67

smackfu · 2024-11-13T14:23:30Z

vitest.config.js

I wonder why this file didn't get flagged by the issue this PR fixes:
americanexpress/eslint-config-amex#130

This file is being ignored currently

#57 (comment)

addressed in 5990a67

Matthew-Mallimo · 2024-11-13T14:57:19Z

tsconfig.json

+      "webworker.importscripts",
+      "scripthost"
+    ],
+    "jsx": "react",


Suggested change

"jsx": "react",

"jsx": "react",

Is this needed? this package doesn't contain any react code

addressed in 5990a67

Matthew-Mallimo · 2024-11-13T14:59:04Z

.github/workflows/release-please.yml

shouldn't this include a build step now?

We already had an existing build step, its covered by:

"prepublishOnly": "npm run build",

Though, personally, I prefer prepack over prepublishOnly

Yes, I would suggest prepack. Otherwise when you run npm pack it doesn't rebuild.

10xLaCroixDrinker · 2024-11-13T18:52:36Z

.babelrc

@@ -1,3 +1,4 @@
 {
-  "presets": ["amex"]
+  "presets": ["amex"],
+  "plugins": ["@babel/plugin-transform-typescript"]


Does babel-preset-amex need TS support?

e.g.

{ "presets": ["amex/ts"] }

honestly, we maybe don't even need a separate file. could just use overrides for .ts and .tsx

10xLaCroixDrinker · 2024-11-13T18:52:55Z

.eslintignore

@@ -1,3 +1,5 @@
 lib
 node_modules
 test-results
+vitest.config.js


Why ignore this?

Aha! That's why it didn't run into this issue:
americanexpress/eslint-config-amex#130

addressed in 5990a67

10xLaCroixDrinker · 2024-11-13T18:54:06Z

package.json

-    "amex-jest-preset": "^7.0.0",
+    "@types/redux-immutable": "^4.0.6",
+    "@typescript-eslint/eslint-plugin": "^6.21.0",
+    "@vitest/coverage-v8": "^2.1.4",
    "babel-preset-amex": "^4.0.3",
    "coveralls": "^3.1.1",
    "eslint": "^8.56.0",
    "eslint-config-amex": "^16.0.0",


Suggested change

"eslint-config-amex": "^16.0.0",

"eslint-config-amex": "^16.2.0",

May as well 🤷🏼‍♀️

addressed in 5990a67

10xLaCroixDrinker · 2024-11-13T18:56:36Z

package.json

@@ -5,27 +5,18 @@
  "main": "lib/index.js",
  "scripts": {
    "clean": "rimraf lib",


Suggested change

"clean": "rimraf lib",

"rimraf": "node -e \"process.argv.slice(1).forEach(path => require('node:fs').rmSync(path, { recursive: true, force: true }));\" --",

"clean": "npm run rimraf lib",

addressed in 5990a67

10xLaCroixDrinker · 2024-11-13T18:56:55Z

package.json

    "redux": "^5.0.1",
    "redux-immutable": "^4.0.0",
-    "rimraf": "^5.0.5"
+    "rimraf": "^5.0.5",


Suggested change

"rimraf": "^5.0.5",

addressed in 5990a67

10xLaCroixDrinker · 2024-11-13T18:57:47Z

package.json

    "babel-preset-amex": "^4.0.3",
    "coveralls": "^3.1.1",
    "eslint": "^8.56.0",
    "eslint-config-amex": "^16.0.0",
+    "eslint-import-resolver-typescript": "^3.6.3",
    "eslint-plugin-jest": "^27.6.3",


Is this still needed?

addressed in 5990a67

10xLaCroixDrinker · 2024-11-13T19:10:04Z

__tests__/__snapshots__/index.spec.ts.snap


-exports[`vitruvius should return an initialState that is acceptable to redux toolkit's configureStore 1`] = `


Intentionally deleted snapshot?

https://github.com/americanexpress/vitruvius/pull/57/files/785fd5ee76a0bcf178d917640bb6905e8a9c0d79#r1841030458

restored now that this test has been restored as per your suggestion

addressed in
5990a67

__tests__/collectBuiltState.spec.ts

10xLaCroixDrinker · 2024-11-13T19:16:15Z

__tests__/fixtures.js

-        other: otherReducer,
-        static: staticReducer,
-      });
-      const store = configureStore({ reducer }, reducer.buildInitialState(locals));


I think it should be:

const store = configureStore({ reducer, preloadedState: reducer.buildInitialState(locals) });

you are right

addressed in 5990a67

10xLaCroixDrinker · 2024-11-13T19:18:12Z

__tests__/fixtures.ts

+  });
+}
+
+export function runVitruviusImmutableTests(


aww, by TS overrides are sooooo fun 🤡

10xLaCroixDrinker · 2024-11-13T19:31:49Z

package.json

-    "prebuild": "npm run test:lint && npm run clean",
-    "build": "babel src -d lib --copy-files",
+    "prebuild": "npm run test:lint",
+    "build": "babel src -d lib --extensions '.ts' --copy-files & tsc",
    "test": "npm run test:lint && npm run test:unit",


Can we implement tsd and test our types?

e.g. https://github.com/pinojs/pino-abstract-transport/blob/main/test/types/index.test-d.ts

The idea is that the types are implicitly tested by the incredibly strict configuration we have for tsc and eslint.

I would be happy to investigate tsd as a follow up piece of work

10xLaCroixDrinker · 2024-11-13T19:34:37Z

A little feedback on the commit message. It is debatable whether this is a feature, but if it is, only the addition of types is a feature and that's all that should be in the head of the message. Details about refactors and tests should be in the body. This will make for a better changelog upon release

code-forger · 2024-12-10T11:46:07Z

@10xLaCroixDrinker

A little feedback on the commit message....

did an amend to fix, look good now?

Refactor all code into typescript

smackfu · 2024-12-11T17:05:51Z

@code-forger this isn't working for me. I'm not getting the types in the package:

As a result getting this error:

src/ducks.ts:1:29 - error TS7016: Could not find a declaration file for module '@americanexpress/vitruvius/immutable'.

code-forger · 2024-12-12T12:50:31Z

@smackfu an npm run build produces this output for me

code-forger · 2024-12-16T09:44:40Z

@smackfu i tested with pack, this was an npm ignore issue, fixed in 7e83db3

code-forger requested review from a team as code owners November 13, 2024 00:44

github-actions bot added the one-app-team-review-requested label Nov 13, 2024

code-forger commented Nov 13, 2024

View reviewed changes

guym4c previously approved these changes Nov 13, 2024

View reviewed changes

smackfu reviewed Nov 13, 2024

View reviewed changes

Matthew-Mallimo reviewed Nov 13, 2024

View reviewed changes

10xLaCroixDrinker reviewed Nov 13, 2024

View reviewed changes

10xLaCroixDrinker requested changes Nov 13, 2024

View reviewed changes

code-forger dismissed guym4c’s stale review via f04d16d December 10, 2024 11:45

code-forger force-pushed the feat/typescript branch from 785fd5e to f04d16d Compare December 10, 2024 11:45

feat(typescript): add types directory with generated types

5990a67

Refactor all code into typescript

code-forger force-pushed the feat/typescript branch from f04d16d to 5990a67 Compare December 10, 2024 12:59

docs(typeing): add docs for how to use typed

d262751

code-forger added 2 commits December 12, 2024 12:50

test(snap): commit snap

dec4806

chore(ignore): unignore types output

7e83db3

chore(types): package.json exports for types

6f3e83b

This was referenced Dec 23, 2024

test(lint): fix lint error after building #55

Closed

chore: limit packed files #56

Closed

		@@ -43,7 +43,7 @@ reducer.buildInitialState = buildInitialState;
		```

		@@ -0,0 +1,14 @@
		import { defineConfig } from 'vitest/config';

	"eslint-config-amex": "^16.0.0",
	"eslint-config-amex": "^16.2.0",

	"clean": "rimraf lib",
	"rimraf": "node -e \"process.argv.slice(1).forEach(path => require('node:fs').rmSync(path, { recursive: true, force: true }));\" --",
	"clean": "npm run rimraf lib",


		exports[`vitruvius should return an initialState that is acceptable to redux toolkit's configureStore 1`] = `

feat(*): typescript prettier vitest #57

Are you sure you want to change the base?

feat(*): typescript prettier vitest #57

Conversation

code-forger commented Nov 13, 2024

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

smackfu left a comment

Choose a reason for hiding this comment

smackfu Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

code-forger Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

code-forger Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

code-forger Dec 10, 2024 • 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

code-forger Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

code-forger Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

code-forger Dec 10, 2024 • 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

code-forger Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

code-forger Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

code-forger Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

code-forger Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

code-forger Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

code-forger Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

code-forger Dec 10, 2024 • 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

10xLaCroixDrinker commented Nov 13, 2024

code-forger commented Dec 10, 2024

smackfu commented Dec 11, 2024

code-forger commented Dec 12, 2024

code-forger commented Dec 16, 2024

smackfu Nov 13, 2024 •

edited

Loading

code-forger Dec 10, 2024 •

edited

Loading

code-forger Dec 10, 2024 •

edited

Loading

code-forger Dec 10, 2024 •

edited

Loading

code-forger Dec 10, 2024 •

edited

Loading

code-forger Dec 10, 2024 •

edited

Loading

code-forger Dec 10, 2024 •

edited

Loading

code-forger Dec 10, 2024 •

edited

Loading

code-forger Dec 10, 2024 •

edited

Loading

code-forger Dec 10, 2024 •

edited

Loading

code-forger Dec 10, 2024 •

edited

Loading

code-forger Dec 10, 2024 •

edited

Loading

code-forger Dec 10, 2024 •

edited

Loading

code-forger Dec 10, 2024 •

edited

Loading