#9430: extracting first util lib #9491
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9491 +/- ##
==========================================
+ Coverage 74.24% 75.72% +1.47%
==========================================
Files 1332 1406 +74
Lines 40817 42439 +1622
Branches 7634 7816 +182
==========================================
+ Hits 30306 32135 +1829
+ Misses 10511 10304 -207 ☔ View full report in Codecov by Sentry. |
Playwright test results
Details
Skipped testschrome › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options |
| @@ -178,6 +185,42 @@ describe("Add/Remove Bricks", () => { | |||
| ).toBeArrayOfSize(initialBricks.length + 1); | |||
| }); | |||
|
|
|||
| test("Add Brick - bad pipeline path error", async () => { | |||
There was a problem hiding this comment.
Added this test just to ensure there was no regression with this existing debug util code.
| @@ -28,7 +28,7 @@ type InvalidPathInformation = { | |||
| * @param path period separated path | |||
| */ | |||
| export function getInvalidPath( | |||
| value: UnknownObject, | |||
| value: Record<string, unknown>, | |||
There was a problem hiding this comment.
UnknownObject comes from a global type def, which we might eventually want to move to the root as well.
There was a problem hiding this comment.
We might also consider using typefest's UnknownRecord instead: https://github.com/sindresorhus/type-fest/blob/main/source/unknown-record.d.ts. There's is maybe technically more accurate — ours only supports string keys vs. strings/symbols/numbers: https://www.totaltypescript.com/concepts/propertykey-type
Although, ours is helpful for distinguishing against mistaken Array uses
| "engine-strict": true, | ||
| "engines": { | ||
| "node": "20.12.0", | ||
| "npm": "10.5.0" | ||
| }, |
There was a problem hiding this comment.
moved to the root package.json
| "compilerOptions": { | ||
| "sourceMap": true, | ||
| "module": "esnext", | ||
| "target": "es2023", |
There was a problem hiding this comment.
Should we consider defining a config these can extend from? E.g., in some libraries we might be able to enforce stricter rules (e.g., noUnusedParameters)
Update: I see you already mentioned the duplication in the PR description
There was a problem hiding this comment.
Yes, I think we should define a base tsconfig. That's the follow-up I mentioned in the PR description
|
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
twschiller
changed the title
| @@ -28,7 +28,7 @@ type InvalidPathInformation = { | |||
| * @param path period separated path | |||
| */ | |||
| export function getInvalidPath( | |||
There was a problem hiding this comment.
NIT: as an aside, given this method uses Formik's getIn vs. lodash's (I'm not sure if there are any differences?) we might consider moving it to a formikDebugUtils module or similar in the future
Or, if there are no differences in getIn implementation, we might consider dropping the Formik dependency from this library in favor of lodash
What does this PR do?
pixiebrix-app) and doesn't have any other dependenciesnxand catch any potential issues.Discussion
nx's build and caching mechanisms. I will look into this in the next set of changes alongside creating a root common ts config.Checklist
For more information on our expectations for the PR process, see the
code review principles doc