-
Notifications
You must be signed in to change notification settings - Fork 6
MEN-8281 - types added to deployments slice #200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
de9306c
to
049540a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces enhanced type definitions across the deployments slice and related thunks with accompanying test updates to reflect these changes. Key updates include adding explicit types (e.g. using createAppAsyncThunk), updating test expectations (e.g. using d3 instead of d1 in some cases), and refining action payload typings to improve type safety and code clarity.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
tests/mockData.js | Added explicit undefined values for filter and group, and introduced a new deployment test entity “d3”. |
packages/utils/src/helpers.test.ts | Updated expected sorting orders in tests to match new deployment IDs. |
packages/store/src/storehooks.test.ts | Adjusted payloads in test actions to align with new types. |
packages/store/src/store.ts | Added type annotations and integrated createAppAsyncThunk for improved async handling. |
packages/store/src/organizationSlice/types.ts | Exported SortOptions for broader use. |
packages/store/src/deploymentsSlice/thunks.ts | Refactored thunks with stronger type definitions and updated API calls with generic types. |
packages/store/src/deploymentsSlice/thunks.test.ts | Updated tests to reflect changes in deployment ID usage and payload structure. |
packages/store/src/deploymentsSlice/reducer.test.ts | Updated reducer tests to use action.type properties. |
packages/store/src/deploymentsSlice/index.ts | Revised slice definitions with full TypeScript support and added explicit payload types. |
packages/store/src/deploymentsSlice/constants.ts | Strengthened immutability hints by adding “as const” to constants. |
packages/store/src/api/general-api.ts | Made minor adjustments to method signatures for consistency. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feel free to take or leave these 👌
const newDeployment = { | ||
newDeployment: { name: 'new Deployment', artifact_name: 'artifact', devices: [Object.keys(defaultState.devices.byId)[0]] }, | ||
hasNewRetryDefault: true | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that the lowest common denominator might be { name: 'new Deployment', artifact_name: 'artifact' }
+ the devices
get overridden in 2 of 3 cases and especially the hasNewRetryDefault
would trigger different actions if retries
would be set, I'd say:
const newDeployment = { | |
newDeployment: { name: 'new Deployment', artifact_name: 'artifact', devices: [Object.keys(defaultState.devices.byId)[0]] }, | |
hasNewRetryDefault: true | |
}; | |
const newDeployment = { name: 'new Deployment', artifact_name: 'artifact' }; |
and then use that in the tests.
@@ -101,7 +129,7 @@ const isWithinFirstMonth = expirationDate => { | |||
return endOfFirstMonth > new Date(); | |||
}; | |||
|
|||
const trackDeploymentCreation = (totalDeploymentCount, hasDeployments, trial_expiration) => { | |||
const trackDeploymentCreation = (totalDeploymentCount: number, hasDeployments: boolean, trial_expiration: string | null) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const trackDeploymentCreation = (totalDeploymentCount: number, hasDeployments: boolean, trial_expiration: string | null) => { | |
const trackDeploymentCreation = (totalDeploymentCount: number, hasDeployments: boolean, trial_expiration?: string) => { |
considering the type for trial_expiration
in TenantV1
const deployment = { | ||
...newDeployment, | ||
id: deploymentId, | ||
devices: 'devices' in newDeployment && newDeployment.devices ? newDeployment.devices.map(id => ({ id, status: 'pending' })) : [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
devices: 'devices' in newDeployment && newDeployment.devices ? newDeployment.devices.map(id => ({ id, status: 'pending' })) : [], | |
devices: newDeployment.hasOwnProperty('devices') ? newDeployment.devices.map(id => ({ id, status: 'pending' })) : [], |
to stay consistent with similar checks in the codebase 👌.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately, hasOwnProperty
is not currently providing the type narrowing, so we would need to alter the interface of the function to something like this:
interface Object {
hasOwnProperty<K extends PropertyKey>(key: K): this is Record<K, unknown>;
}
'prop' in obj
is a current suggested type guard for typescript
I am ok with both approaches, but I would probably change it in a separate task (maybe the one for type improvement)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😞 nah, no need to change this, but I would still consider the in
usage risky/ a bad practice... and yes, that might be a good addition to the type improvement to ensure we don't rely on in
if possible/ we have types used more broadly (and then maybe microsoft/TypeScript#44253 (comment) to not rely on it being a Record
or unknown
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated QA-1031 to include this
const { previousPhases = [], retries: previousRetries = 0 } = getGlobalSettings(getState()); | ||
let newSettings: any = { retries: hasNewRetryDefault ? retries : previousRetries, hasDeployments: true }; | ||
if (phases) { | ||
//TODO: fix backend and helper package types incompatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you create a follow up task for this, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ticket: MEN-8281 Changelog: None Signed-off-by: Mikita Pilinka <[email protected]>
Ticket: MEN-8281 Changelog: None Signed-off-by: Mikita Pilinka <[email protected]>
Ticket: MEN-8281 Changelog: None Signed-off-by: Mikita Pilinka <[email protected]>
Ticket: MEN-8281 Changelog: None Signed-off-by: Mikita Pilinka <[email protected]>
Ticket: MEN-8281 Changelog: None Signed-off-by: Mikita Pilinka <[email protected]>
Ticket: MEN-8281 Changelog: None Signed-off-by: Mikita Pilinka <[email protected]>
No description provided.