Bump typescript to 4.6.4

[gulderov]

Description

Bump typescript to 4.6.4

Issues Resolved

closes #888
closes #887

Check List

• All tests pass
  • yarn lint
  • yarn test-unit
• Update CHANGELOG.md
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[BSFishy]

I don't love adding new code with any. Could this be cast as object or unknown instead?

[gulderov]

Maybe, we could consider completely removing this. Please look at the follow up issue #886

[SergeyMyssak]

Alternatively, we can add the following code to index.d.ts

interface Window {
  MSInputMethodContext?: any; // is IE11
}

interface Document {
  documentMode?: any; // is IE11
}

[gulderov]

Good point about fixing it by adding to global types. The usage of any is common to highlight areas of uncertainty or complexity in typescript. It is usually recommended to avoid it if possible

In this case it should be safe to mark some pain point by as any. It could be more scary to cleanup index.d.ts in future, than cleanup local usage of any. My preference is to keep theindex.d.ts nice and clean.

On other hand I cant see better cast here and we have issue #886 for refactoring

[BSFishy]

Love that we're able to get rid of this @ts-ignore

[BSFishy]

Why is onPinClick removed here? From a brief look at the code, it seems like it should be null checked

[gulderov]

The onPinClick is defined as required prop for OuiPinnableListGroup component. Typescript will give error, if onPinClick is not provided. So no extra validation needed. I hope we can benefit from type checking here

[AMoo-Miki]

Compile-time warning cannot take the place of proper guarding. There are many scenarios that TS will not be able check during compiling.

That said, onPinClick check shouldn't be up there but should be added down.

if (onPinClick) itemProps.extraAction.onClick = () => onPinClick(item);

[AMoo-Miki]

I committed the above suggestion.

[BSFishy]

Why do we want to keep it as any? Is there a significant downside of using unknown? Will it involve a substantial amount of extra work? Could it be done in a followup task?

[gulderov]

Typescript can enhance code quality, but it is also important to keep balance between type safety and ease of use

The useUnknownInCatchVariables option is useful because it enforces a higher level of type safety in catch blocks. However, enabling this option can also lead to additional complexity in code. We would need to add extra type checking code in each catch block.

Let me show on examples

It is common to expect catch variable as instance of Error:

try {
  throw Error('error message');
} catch (error) {
  console.log(error.message); // => LOG: 'error message'
}

but it is not always the case:

try {
  throw 'error message';
} catch (error) {
  console.log(error.message); // => LOG: undefined
}

useUnknownInCatchVariables is intended to catch errors like this:

try {
  throw 123;
} catch (error) {
  if (err instanceof Error) {
    console.log(err.message);
  } else if (typeof err === 'string') {
    console.log(err);
  } else {
    console.log('Unexpected error')

  }
}

or:

try {
  throw 123;
} catch (error) {
  const e = error instanceof Error ? error : Error( `Unexpected Error: ${error}`);
   console.log(e.message);
}

Same time it is much easier to avoid checks by ignoring types:

try {
    throw new OwnError(123);
} catch (error: unknown) {
    console.log((error as OwnError).message);
}

or even:

try {
  throw new Error(123);
} catch (error : any) {
   console.log(error.message);
}

While enabling useUnknownInCatchVariables could potentially catch errors, it might also introduce extra complexity into codebase. I am cautious about making this change, especially since the useUnknownInCatchVariables is enabled by default.

I would be happy to make this change, if benefits of enabling useUnknownInCatchVariables outweigh potential downsides. Please let me know if you want to proceed with this and I will create a followup task

[SergeyMyssak]

It's a good idea to reduce complexity and remove conditions inside the catch block, but I think type safety and robust error handling have a higher weight than reducing complexity like that, so for me, it's better to use unknown rather than any for error variables in catch clauses

[gulderov]

There seems to be some confusion regarding this change. I have created issue #887 to do refactoring with useUnknownInCatchVariables in separate task. It will help to keep this pull request limited.

Could you please review it @BSFishy @SergeyMyssak ?

[SergeyMyssak]

I'm wondering if we can avoid this change? I see that you have created an issue to remove it

[gulderov]

useUnknownInCatchVariables is not in tsconfig anymore. We have changes in this pull request to close #887

[gulderov]

The @typescript-eslint/eslint-plugin: "^6.0.0" requires at least node v16, but .nvmrc contains 14.20.0

[SergeyMyssak]

Alternatively, we can add the following code to index.d.ts

interface Window {
  MSInputMethodContext?: any; // is IE11
}

interface Document {
  documentMode?: any; // is IE11
}

[SergeyMyssak]

Are you sure we should put onClick after {{...(rest as PropsForButton)}? I understand that TS gives an error there, but it would still be good to understand why it started giving it

[gulderov]

Typescript highlights that onClick is specified more than once, so usage will be overwritten

This is due usage of as PropsForButton, which reintroduces the onClick while it is already picked

  ButtonHTMLAttributes<HTMLButtonElement>,
  'onClick' | 'type'
> & {
  onClick: MouseEventHandler<HTMLButtonElement> | undefined;
};

Provided solution is easer than {...(rest as Omit<PropsForButton, 'onClick'}or refactoring of the PropsForButton or keeping onClick in rest

[SergeyMyssak]

I don't really understand why this interface is written as it is, I think it could be simplified and the TS error would disappear:

type PropsForButton = Omit<ButtonHTMLAttributes<HTMLButtonElement>, 'type'>;

[gulderov]

Typing is changed to suggested

[SergeyMyssak]

It's a good idea to reduce complexity and remove conditions inside the catch block, but I think type safety and robust error handling have a higher weight than reducing complexity like that, so for me, it's better to use unknown rather than any for error variables in catch clauses

[SergeyMyssak]

I don't really understand why this interface is written as it is, I think it could be simplified and the TS error would disappear:

type PropsForButton = Omit<ButtonHTMLAttributes<HTMLButtonElement>, 'type'>;

[SergeyMyssak]

I'm wondering if we can avoid this change? I see that you have created an issue to remove it

[SergeyMyssak]

@gulderov well done, thank you for your contribution! All that's left is to fix the merge conflicts.

@BSFishy, are we going to backport this PR to 1.x?

[BSFishy]

@BSFishy, are we going to backport this PR to 1.x?

Yep, I don't see any public API changes, so this should be good to go.

I'd also like to get another pair of eyes or two on this, as its blast radius might be a little bit larger than we usually have. Pinging people now

[joshuarrrr]

LGTM

[joshuarrrr]

It looks like we're changing the return type to string from string | undefined - is this intentional, and does it have any effect in caller contexts?

[AMoo-Miki]

While i think it will impact the caller contexts, i believe the previous behavior of returning undefined was wrong.

[AMoo-Miki]

Let's strengthen this a bit:

     const message = (e instanceof Error ? e.message || e.code : String(e)).trim();

      error(
        `Invalid value \`${expression}\` set for field \`${field}\`${message ? ` - ${message}` : '' }`,
        location
      );

[AMoo-Miki]

I committed a nicer version of the above suggestion.
PS, tsc didn't like that. Will adjust it.

[joshuarrrr]

Thanks @gulderov ! This was a very valuable contribution!