Skip to content

Fix typescript errors when using Stacks package #1012

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

Closed
wants to merge 1 commit into from

Conversation

mukunku
Copy link
Collaborator

@mukunku mukunku commented Jul 13, 2022

Summary

We are using the Stacks package in our solution in Visual Studio but there are these typescript compilation related errors that show up within the IDE:
image

I'm not sure if my proposed changes are the best way to address this but having those errors in my Visual Studio can be quite annoying and I haven't been able to figure out a way to hide/ignore them. This PR is my naïve attempt at trying to get rid of those errors.

If I make the proposed changes on my machine the errors go away:
image
But this doesn't solve it for others and the issue would most likely reoccur if we were to update the package and the files are overwritten.

@mukunku mukunku added the help-wanted Issues a person would like or needs help with label Jul 13, 2022
@mukunku mukunku requested review from b-kelly and dancormier July 13, 2022 15:41
@netlify
Copy link

netlify bot commented Jul 13, 2022

Deploy Preview for stacks ready!

Name Link
🔨 Latest commit 4440de5
🔍 Latest deploy log https://app.netlify.com/sites/stacks/deploys/62cee7b44f02cc00081d73f5
😎 Deploy Preview https://deploy-preview-1012--stacks.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@dancormier
Copy link
Contributor

Thanks @mukunku! FYI, I opened a PR with a similar goal recently. I'll take a look at your changes here soon.

@@ -142,7 +142,7 @@ function getCellSlot(cell: HTMLTableCellElement): number {
if (!(cell.parentElement && cell.parentElement.parentElement instanceof HTMLTableSectionElement)) {
throw "invalid table"
}
const result = buildIndexOrGetCellSlot(cell.parentElement.parentElement, cell);
const result = buildIndexOrGetCellSlot(cell.parentElement.parentElement as any as HTMLTableSectionElement, cell);
Copy link
Collaborator

Choose a reason for hiding this comment

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

any is a bit of an early escape hatch here. You can use a type predicate/guard to safely convince TS you are looking at the right type.

We're supposedly already doing that above with this check:

if (!(cell.parentElement && cell.parentElement.parentElement instanceof HTMLTableSectionElement)) {
    throw "invalid table"
}

But perhaps TS remains unconvinced?

We can add this type guard:

function isHTMLTableSectionElement(el: HTMLElement | null | undefined): el is HTMLTableSectionElement {
    return el instanceof HTMLTableSectionElement;
}

You then rewrite like this:

function getCellSlot(cell: HTMLTableCellElement): number {
    const grandParent = cell.parentElement?.parentElement
    if (!isHTMLTableSectionElement(grandParent)) {
        throw "invalid table"
    }
    const result = buildIndexOrGetCellSlot(grandParent, cell);
    if (typeof result !== "number") {
        throw "shouldn't happen"
    }
    return result
}

Copy link
Contributor

Choose a reason for hiding this comment

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

But perhaps TS remains unconvinced?

Yup, this is known TS behavior, see microsoft/TypeScript#10421. If these objects/values are coming from built-in browser functions or we're positive there's no way the wrong type could exist here. Then we can create a utility (assumeType<T>()) to make it easy for us to fix this in the future,

function assumeType<T>(x: unknown): asserts x is T {
    return; // ¯\_(ツ)_/¯
}

function processGeometry(obj: Geometry): void {
    if ("width" in obj) {
        assumeType<AARect>(obj);
        let width = obj.width;
        // ...
    }
    if ("halfDimX" in obj) {
        assumeType<AABox>(obj);
        let halfDimX = obj.halfDimX;
        // ...
    }
    else if ("radius" in obj) {
        assumeType<Circle>(obj);
        let radius = obj.radius;
        // ...
    }
    // And much more...
}

Source: microsoft/TypeScript#10421 (comment)

Copy link
Collaborator Author

@mukunku mukunku Jul 13, 2022

Choose a reason for hiding this comment

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

The double cast to any then to the element is definitely an ugly band-aid.

I tried the type guard approach but I'm still getting some errors from the typescript compiler:
image

The assume type approach also didn't work for me. Not sure if I'm doing something incorrectly:
image

Copy link
Collaborator

@KyleMit KyleMit Jul 13, 2022

Choose a reason for hiding this comment

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

Huh, I can't repro your error @mukunku, either in my editor or in this TS Playground Demo

If I control click on HTMLTableSectionElement it provides the following source code:

/** Provides special properties and methods (beyond the HTMLElement interface 
  it also has available to it by inheritance) for manipulating the layout and 
  presentation of sections, that is headers, footers and bodies, in an HTML table. 
*/
interface HTMLTableSectionElement extends HTMLElement {}

So it does seem fair that it should be assignable in a predicate.

But I believe the squiggles in your screenshot; I just wonder where they're coming from.

@mukunku
Copy link
Collaborator Author

mukunku commented Jul 13, 2022

Thanks @mukunku! FYI, I opened a PR with a similar goal recently. I'll take a look at your changes here soon.

Thanks @dancormier. Out of curiosity I tried using the versions of those two files you have in your PR to see if the error was already fixed however the errors still persisted on my end. I'm happy to close this PR out if you'd rather add it to your changes. Let me know!

@dancormier
Copy link
Contributor

dancormier commented Jul 13, 2022

Out of curiosity I tried using the versions of those two files you have in your PR to see if the error was already fixed however the errors still persisted on my end.

Ah, I'm a fool! s-table.ts in my PR has no type-related fixes. 🤦 I was operating under the (incorrect) assumption that all errors would surface in Stacks standalone. That being said, I don't get any errors on .s-tooltip.ts in Stacks or Core 🤔

I'm happy to close this PR out if you'd rather add it to your changes. Let me know!

If the errors are resolved here, this PR should be good to merge in and #1009 can be merged separately.

I'm gonna poke around at that s-table.ts fix in ~20 minutes and report back.

@dancormier
Copy link
Contributor

Closing in favor of https://github.com/StackEng/StackOverflow/pull/12361

@dancormier dancormier closed this Jul 13, 2022
@b-kelly b-kelly deleted the suppress-type-casting-warning branch January 18, 2023 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help-wanted Issues a person would like or needs help with
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants