Skip to content

refactor(@angular-devkit/schematic): replace any with unknown#18541

Closed
santoshyadavdev wants to merge 1 commit intoangular:masterfrom
santoshyadavdev:refatcor/change-the-type-to-unknown
Closed

refactor(@angular-devkit/schematic): replace any with unknown#18541
santoshyadavdev wants to merge 1 commit intoangular:masterfrom
santoshyadavdev:refatcor/change-the-type-to-unknown

Conversation

@santoshyadavdev
Copy link
Contributor

No description provided.

@santoshyadavdev santoshyadavdev force-pushed the refatcor/change-the-type-to-unknown branch from 4c23dbc to 6f43fc0 Compare August 15, 2020 08:16
@santoshyadavdev santoshyadavdev force-pushed the refatcor/change-the-type-to-unknown branch from 6f43fc0 to 7b177a2 Compare August 22, 2020 09:30
const kind = act && act.kind;

return typeof action === 'object'
&& typeof act.hasOwnProperty('kind')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
&& typeof act.hasOwnProperty('kind')

export function isAction(action: any): action is Action { // tslint:disable-line:no-any
const kind = action && action.kind;
export function isAction(action: unknown): action is Action {
const act = action as Action;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In all fairness I am not a big fan of this casting here, as this is hiding potential errors.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like seems to be more "correct", but wondering if we should just stop at hasOwnProperty since it's the object has those 3 props it's should be an action.

export function isAction(action: unknown): action is Action {
  if (
    action
    && typeof action === 'object'
    && action.hasOwnProperty('path')
    && action.hasOwnProperty('id')
    && action.hasOwnProperty('kind')
  ) {
    const { kind, id, path } = action as { id: unknown, path: unknown, kind: unknown };
    if (
      typeof kind === 'string'
      && ['c', 'o', 'r', 'd'].includes(kind)
      && typeof id === 'number'
      && typeof path === 'string'
    ) {
      return true;
    }
  }

  return false;
}

//cc @clydin what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

based on the usage of the function, I think we should consider deprecating it and leaving it as-is for now. I put together a PR that removes the only use internally: #18637

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@santoshyadavdev, mind opening a separate PR to deprecate the this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh cool @alan-agius4 will do that today.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alan-agius4 please see #18727, should I close this one.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I’ll close it for you.

@alan-agius4 alan-agius4 closed this Sep 8, 2020
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants