3
\$\begingroup\$

This is my first attempt in converting a pure javascript to typescript. Any comment?

const apiUrl = 'https://localhost:123'

export function webApiUrl() {
    return apiUrl;
}

export function downloadPdf(id : number, fileName: string) {
    const spinner: HTMLElement = document.getElementById('spinner') as HTMLImageElement
     
    spinner.style.display = 'block'

    fetch(`${apiUrl}/api/file/download?fileDocumentId=${id}`).then(resp => resp.arrayBuffer()).then(resp => {

        // set the blog type to final pdf
        const file = new Blob([resp], { type: 'application/pdf' });

        // process to auto download it
        const fileURL: string = URL.createObjectURL(file);
        const link: HTMLAnchorElement = document.createElement('a');
        link.href = fileURL;
        link.download = fileName;
        link.click();
        spinner.style.display = 'none'
    });
}

I think the only code I can't figure out how to convert over is the blob type.

\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Use explicit type annotation only when necessary In the vast majority of cases, TypeScript can infer the type of an expression automatically, without you having to note it manually. It's probably better and easier to let TS take care of it - it makes for less code to read and write. If you're ever not sure what sort of type an expression is, use an IDE which can tell you (like VSCode, which works really well with TypeScript - just hover over an expression and it'll tell you what type TypeScript has inferred for it).

For example:

const fileURL: string = URL.createObjectURL(file);
const link: HTMLAnchorElement = document.createElement('a');

can be

const fileURL = URL.createObjectURL(file);
const link = document.createElement('a');

IMO, TypeScript shines most when it looks almost identical to JavaScript, with the added bonus of warning you when you're doing something that's type-unsafe.

I think the only code I can't figure out how to convert over is the blob type.

Its type is a Blob, which you can see in VSCode (or any other good TS-aware IDE):

enter image description here

But, as said, better to let TS infer it automatically; no need for const file: Blob =.

Prefer generics over type assertion Generics are the most elegant way to denote the type of a particular expression, when the type isn't set in stone by the function signature. The as keyword, in contrast, is less type-safe: as tells TypeScript "I know exactly what I'm doing, ignore whatever type this value was determined to be previously and assume that it's of THIS type instead".

For example, the following code does not throw a TS error, despite the as clearly being nonsensical:

const foo = {
    bar: 'bar'
} as {
    fn?: () => Promise<Array<string>>
};

as isn't as bad as any, but they both should be avoided when possible.

In your case, getElementById is not generic, unfortunately, but querySelector is. You can change:

const spinner: HTMLElement = document.getElementById('spinner') as HTMLImageElement

to

const spinner = document.querySelector<HTMLElement>('#spinner')!;
// you could also use <HTMLImageElement> if you needed image element specific types

Generics are essentially type arguments that come in <> brackets, before the expression arguments inside the parentheses of the function call. Above, <HTMLImageElement> tells querySelector: "This selector, if it matches, will match an HTMLImageElement." This is preferable to as because only the types that extend Element can be passed to querySelector.

Function name Functions should describe actions, or what they do. webApiUrl might be more precisely named getApiUrl. (Or, just export the URL string alone: export const apiUrl = '...';, there doesn't seem to be any need for the function)

Error handling If the fetch fails, no indication is given to the user or to other parts of the program. Whenever you have a Promise, you should almost always make sure that possible errors inside it get handled. TSLint rule: no-floating-promises.

While you could add a .catch onto the fetch Promise chain:

    spinner.style.display = 'none'
})
.catch((error) => {
    // handle errors
});

It might be better to let the caller handle errors as needed. Change:

fetch(`${apiUrl}/api/...

to

return fetch(`${apiUrl}/api/...

so that each caller can do downloadPdf(id, fileName).catch(handleErrors) individually.

Semicolons Some of your lines are missing semicolons. This is stylistically inconsistent and can lead to odd bugs if you forget a semicolon where it's required. Add the missing semicolons, and consider using a linter which warns you when they're missing. (The rationale behind using a linter is similar to the rationale behind using TypeScript - turn annoying, hard-to-encounter, sometimes-hard-to-debug runtime errors into trivially fixable compile-time errors.)

\$\endgroup\$
5
  • \$\begingroup\$ Thanks a lot for the comprehensive code review. May I know what is the purpose of the last symbol ! in const spinner = document.querySelector<HTMLElement>('#spinner')!;? \$\endgroup\$
    – Steve Ngai
    Nov 4, 2020 at 2:19
  • 1
    \$\begingroup\$ That tells TS that the expression definitely isn't null nor undefined. \$\endgroup\$ Nov 4, 2020 at 2:21
  • \$\begingroup\$ It's not a good practice using !, you can't be sure that the spinner element always exists in the HTML (what if something else changed HTML?). So you should always check if it is defined. If you wish to have a nice TS code, you can use Optional Chaining. \$\endgroup\$
    – KiraLT
    Nov 19, 2020 at 19:48
  • \$\begingroup\$ @KiraLT If the script writer has every reason to believe that the element absolutely should exist when the script runs, using ! should be fine. If there's really a possibility that the element wouldn't exist, then yes, you'd want to check that it does first (but there's no indication that this is one of those situations). Ideally, an even better approach would be a framework like React that tightly couples the DOM elements with the state of the application, so that the existence of one without the other is inconceivable. \$\endgroup\$ Nov 19, 2020 at 19:53
  • \$\begingroup\$ From my experience, I can say that even if you think that at the moment it should always exist, the future can prove you wrong and for a simple mistake, your whole app will crash instead of just a loader. In very rare cases maybe you can use it, but it would be best to always handle all edge cases, especially if it won't cause inconvenience. As for react, life is not always is so nice, that you can choose which technology you can use. \$\endgroup\$
    – KiraLT
    Nov 19, 2020 at 20:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.