Convert 6 utils to TypeScript #1023

dhruvkb · 2022-02-28T14:26:24Z

Fixes

Works on milestone 10 i.e. TypeScriptification

Fixes #922
Fixes #924
Fixes #925
Fixes #926
Fixes #936
Fixes #938

Description

This PR

converts a number of utilities to TypeScript
updates usages of TS-ified utilities
adds a lint rule for proper TSDoc comments
converts the Nuxt config to TS and fixes type mismatches
refactors code as needed

Testing Instructions

Passing CI should be sufficient imo. You can try checking out the PR, running the dev server and playing with it for a bit. Repeat with the Storybook.

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

sarayourfriend

Looking great so far! Will happily approve once un-drafted 🎉

src/utils/decode-data.ts

sarayourfriend · 2022-02-28T14:59:16Z

src/utils/resampling.ts

+ * @param data - the list of data points to interpolate
+ * @param threshold - the number of expected data points from the array


It'd be nice to add some doc linting to either enforce or disallow the - syntax in param descriptions. This PR uses both with and without it. I'm sure there's inconsistency with this in the rest of the codebase as well.

Added this in 6a3b521. I've currently restricted this rule to only apply to .ts files.

If you don't mind, it'd probably be better to add it to #999 or just put it into a separate PR entirely. I'd like to go ahead and apply the rules to JS, TS and Vue SFCs as well in one go.

Adding this to JS and Vue is leading to a lot of errors because of the typedef and JSDoc comments that are not compatible with TSDoc. It might be good to re-enable it for JS after a major chunk of the migration is complete.

Yeah, I think it'd be better to use a generic JSDoc one than a TSDoc thing as TSDoc is actually a specific separate standard than JSDoc with different rules in some cases.

dhruvkb · 2022-02-28T15:34:57Z

@sarayourfriend if that's okay I'm planning to hit all the utils in one go and reduce the overhead of approvals, merges and conflicts.

sarayourfriend · 2022-02-28T17:53:20Z

@dhruvkb That might be fine for most of the utils, but I'd encourage splitting anything that has any runtime changes or substantial type additions into a separate PR, especially if the module isn't well unit tested. In my experience in adding types to various Gutenberg packages it was actually much faster go module by module mostly because it made it far easier for reviewers. If converting to TS just means renaming the file and adding parameter and return types to a function and nothing else then it's probably safe to batch those PRs. Otherwise the burden for the reviewer goes up a lot and I'd worry that you'd get stalled just waiting for reviews on a big PR. (Not to mention a bigger PR actually increases the merge conflict potential)

sarayourfriend · 2022-02-28T18:02:15Z

src/utils/warn.ts

+export const getLogger = (level: 'log' | 'warn' | 'error') =>
+  process.env.NODE_ENV !== 'production'
+    ? console[level]
+    : () => {
+        // do nothing
+      }


Unfortunately this makes the types slightly worse than they were before. To get the right types we'll need to do something like this:

import { isNotProd } from '~/utils/dev' type Console = typeof console export const getLogger = <T extends keyof Console>(level: T): Console[T] => isNotProd ? console[level] : (() => { // do nothing }) as Console[T] export const warn = getLogger('warn') export const log = getLogger('log')

We can probably also rename the module to console if it's going to include more than just warn 😁

That makes sense, updated in 3ad59c2.

@dhruvkb did you miss #1023 (comment) or was it something you think we shouldn't do?

This is slightly confusing because I'm still getting the right types. If I import log or warn from this module they show the same type signature as console.log and console.warn.

This is the difference I see:

https://www.typescriptlang.org/play?#code/CYUwxgNghgTiAEYD2A7AzgF3gSzQBRiWAC54AjJJCEKFAbgCgHl0sBzEDAGSTY5ngBeeAAoA1iACepAOQReM+AB94MgO6wUilTIxQy1GQEohAPgbxL8AIS4CReAH5EqNFRABtCZIC68UiIiJoKm8ADeAL5GTCyY8PJsQvAc3Lz8InIK0QD02VbwAHqOMa5YGjAoSSk8fCAwGeVaOXlWRSWs8HoGCMLVaXUZXYbN+W3tcQBm2AAeIMAA4pw1-EkAPAAq8CDTGCAowGjw3kgT8ADCru6m4lKk60akF+juHut+IRZWtviEwE4uz2oXikfgCQTM4Si8CghyebiBbyYzFK8V4ACYklNZgslv16pk2MYGLl8oVisiOo0McIsXNFqlavjGkSSa1ybEsEMQNT4LScQz0rp9MNiS1LG0gA

I'm not sure exactly why it squashes everything to (...data: any[]) => void. I think because it's merging the empty function and the actual console function's type? But it doesn't fix it if you cast the empty function either so I don't think that's why really:

https://www.typescriptlang.org/play?#code/CYUwxgNghgTiAEYD2A7AzgF3gSzQBRiWAC54AjJJCEKFAbgCgHl0sBzEDAGSTY5ngBeeAAoA1iACepAOQReM+AB94MgO6wUilTIxQy1GQEohAPgbxL8AIS4CReAH5EqNFRABtCZIC68UiIiJoKm8ADeAL4mUGjwGJIADiBIAGYu6O4e8Ump8N4+TCyY8PJsQvAc3Lz8InIKRgwA9I1W8AB6joWuWBowKOWVPHwgMLW9Wg3NrR1drHH61AOcQzW6CyDGTS1WM7PFKdgAHiDAAOLL1SPlADwAKvAghxggKMCx3rkAwq7upuJSpFuRlI3wy1A8tz8IQsVls+EIwCc6Tc4Py-lEQTM4Si8Bi8FBKM8kKYzG6JV4ACZygdjmcLsNRnU2JsptNOqS5uMqcIaSdzlUGWNNCztpZdkUsHoDCBufBeXSBaspYZJqL2p0gA

Anyway, it's not a big deal.

dhruvkb · 2022-02-28T18:09:26Z

@sarayourfriend you're right. In that case, I'll stop this PR here and open it to review.

sarayourfriend · 2022-02-28T18:11:13Z

Dockerfile

@@ -76,7 +76,7 @@ COPY pnpm-lock.yaml .
 COPY .npmrc .

 # copy the nuxt configuration file
-COPY --from=builder /usr/app/nuxt.config.js .
+COPY --from=builder /usr/app/nuxt.config.ts .


😱 For some reason I didn't actually think we could use TypeScript for the nuxt config until Nuxt bridge/Nuxt 3 but this is great!!

Question: if the previous stage is the builder and its entrypoint is npm run build, does that means that we shouldn't have any .ts file at this point?

That's a good question 🤔 The Nuxt config is used both to generate the Webpack config but also to configure some runtime properties for Nuxt, so it's used both for building and for runtime. Nuxt's runtime doesn't use TypeScript though, at least not in Nuxt 2, I don't think. If this is working I'm not entire sure why unless the @nuxtjs/typescript module added support for it.

Ok I understand now, I went to the docs, and npm start can differ its behavior depending the way you build the application (nuxt build).

Since the deployment target is not being specified in the config file, then the default value is server which means, we need Node.js to run the application from the generated .nuxt folder.

nuxt start - Start the production server (after running nuxt build). Use it for Node.js hosting like Heroku, Digital Ocean, etc.

That means, at this point, if we want to keep using typescript (after building the application from the previous stage) we will require ts-node. Or, my preferred option, as you said Sara, don't convert the config file so it can be read by Node.js runtime without any external dependency.

Yeah, I'd prefer if we kept it as TS for now until we move to Nuxt bridge. To use TS for this properly it looks like we'd have to bring in another module for it: https://typescript.nuxtjs.org/guide/runtime/

We can always add type-checking to the file even if it stays as JSDoc though; it just can't consume actual TS files (which means anything we use in the nuxt.config.js needs to stay as regular JS with JSDoc types instead of getting converted to TypeScript).

If we don't use TS for nuxt-config, we cannot import the TypeScript versions of the utils node-env, env and sentry-config in it. I'm not entirely sure why making the nuxt-config file TS works but if I change it to JS, it can no longer import any TS files.

JS files can't import TS files without the JS files being compiled first. Because a .js nuxt.config doesn't get compiled, it won't be able to import TS directly I guess (because it's not importing anything from the compiled bundle).

Considering the e2e tests are passing (they run on the pnpm build && pnpm start version of the app, for @rbadillap's context) I guess it's safe.

Also don't know why it's working though (which scares me a little bit 😅)

src/locales/scripts/types.d.ts

src/utils/dev.ts

sarayourfriend · 2022-02-28T18:17:58Z

src/utils/env.ts

+export const env: Record<string, string> = {
  apiUrl: process.env.API_URL ?? 'https://api.openverse.engineering/v1/',
  filterStorageKey: 'openverse-filter-visibility',
  notificationStorageKey: 'openverse-show-notification',
-  enableInternalAnalytics: stringToBoolean(
-    process.env.ENABLE_INTERNAL_ANALYTICS
-  ),
+  enableInternalAnalytics: process.env.ENABLE_INTERNAL_ANALYTICS ?? 'false',
 }


Let's use as const here: it's perfect for this kind of thing. Record<string, string> is actually even worse than what TypeScript infers as it swallows all the key names. as const is the best though as it will turn constant strings into constant string types instead of just string. Changing enableInternalAnalytics to a string boolean representation instead of just a plain boolean also strikes me as a step backwards, what was the reason for that?

This is because Nuxt only accepts the env as a mapping of string keys and string values which makes sense as env vars are always strings. Where we actually use enableInternalAnalytics in store/usage-data.js, we are already converting the string to boolean.

Changed to as const in 5acea54.

Makes sense! I wonder what a good way to enforce that would be 🤔 I guess Record<string, string> would be it but having the keys swallowed is a bit of a bummer 😕

We wouldn't need to enforce this, tsc will raise an error if we use anything other than a string as the value. Also using as const with it works.

sarayourfriend · 2022-02-28T18:20:04Z

tsconfig.json

@@ -46,12 +46,10 @@
    "src/data/api-service.js",
    "src/data/usage-data-service.js",
    "src/store/user.js",
-    "src/utils/decode-data.js",


To get nuxt.config.ts included in the type checking we'll need to change the first entry in includes to just **/*.ts I think. Right now it's only type checking stuff inside the src file (which isn't good considering we'll be adding TypeScript to the test directory as well with the visual regression stuff.

sarayourfriend

LGTM. Still not sure about the TSDoc linting instead of just JSDoc, would like others to give input there. The console types I'd personally like changed but are also not a blocker.

sarayourfriend · 2022-03-01T11:54:12Z

.eslintrc.js

-  plugins: ['@typescript-eslint', 'vue', 'vuejs-accessibility', 'unicorn'],
+  plugins: [
+    '@typescript-eslint',
+    'eslint-plugin-tsdoc',


I still think we might want to use https://www.npmjs.com/package/eslint-plugin-jsdoc instead of a TSDoc one. We have no real idea of how long Vue files will be excluded from TS and it'd be nice to have consistency across the stack with our JSDoc comments.

Let's address the JSDoc changes in #1029.

sarayourfriend · 2022-03-01T11:56:16Z

src/components/VIcon/VIcon.vue

-      type: process.env.NODE_ENV === 'test' ? Object : String,
+      type: isTest ? Object : String,


I would like to check that dead code elimination still works when you don't inline the NODE_ENV check. I don't think we have environment based dead code currently but it would be good to add that. We might end up having to revert these checks back to the inline but I'm not sure.

I'll make a new issue for this, it seems that this should work because it'll evaluate to

if (false) { // ... }

in the compiled JS.

Opened #1030.

obulat

Exciting changes! Thank you for keeping this PR small. With all of the console changes it already looks kind of intimidating :)

Convert the resampling util to TypeScript

f29cff1

dhruvkb added 🟨 priority: medium Not blocking but should be addressed soon ✨ goal: improvement Improvement to an existing user-facing feature 🤖 aspect: dx Concerns developers' experience with the codebase labels Feb 28, 2022

Convert the decode-data util to TypeScript

be13af5

sarayourfriend reviewed Feb 28, 2022

View reviewed changes

Convert the warn util to TypeScript and update usages

bb215be

dhruvkb added 4 commits February 28, 2022 19:48

Lint TypeScript files for TSDoc compliance

6a3b521

Convert the dev util to TypeScript and update usages

94f7dc4

Avoid empty interface

9edcc69

Use TS for the Nuxt config

4249d80

dhruvkb force-pushed the tsify-utils branch from 8e6d848 to 4249d80 Compare February 28, 2022 16:37

Change string-to-boolean util to TypeScript

0b4555e

sarayourfriend reviewed Feb 28, 2022

View reviewed changes

dhruvkb marked this pull request as ready for review February 28, 2022 18:09

dhruvkb requested a review from a team as a code owner February 28, 2022 18:09

dhruvkb requested review from obulat and sarayourfriend February 28, 2022 18:09

Rename warn.ts to console.ts

3ad59c2

sarayourfriend reviewed Feb 28, 2022

View reviewed changes

dhruvkb added 3 commits February 28, 2022 22:56

Change first letter of interface to uppercase

16cd2c9

Use positive form of boolean

c40b471

Use as const to use the narrowest type for env

5acea54

dhruvkb force-pushed the tsify-utils branch from 27920af to 5acea54 Compare February 28, 2022 19:09

Include TypeScript files outside src

0b0bc48

sarayourfriend approved these changes Mar 1, 2022

View reviewed changes

obulat approved these changes Mar 1, 2022

View reviewed changes

dhruvkb changed the title ~~Convert utils to TypeScript~~ Convert 6 utils to TypeScript Mar 1, 2022

dhruvkb merged commit 49b54b5 into main Mar 1, 2022

dhruvkb deleted the tsify-utils branch March 1, 2022 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert 6 utils to TypeScript #1023

Convert 6 utils to TypeScript #1023

dhruvkb commented Feb 28, 2022 •

edited

Loading

sarayourfriend left a comment

sarayourfriend Feb 28, 2022

dhruvkb Feb 28, 2022 •

edited

Loading

sarayourfriend Feb 28, 2022

dhruvkb Feb 28, 2022 •

edited

Loading

sarayourfriend Feb 28, 2022

dhruvkb commented Feb 28, 2022

sarayourfriend commented Feb 28, 2022 •

edited

Loading

sarayourfriend Feb 28, 2022 •

edited

Loading

sarayourfriend Feb 28, 2022

dhruvkb Feb 28, 2022

sarayourfriend Feb 28, 2022

dhruvkb Mar 1, 2022 •

edited

Loading

sarayourfriend Mar 1, 2022

dhruvkb commented Feb 28, 2022

sarayourfriend Feb 28, 2022

rbadillap Feb 28, 2022

sarayourfriend Feb 28, 2022 •

edited

Loading

rbadillap Feb 28, 2022 •

edited

Loading

sarayourfriend Feb 28, 2022

dhruvkb Feb 28, 2022

sarayourfriend Feb 28, 2022

sarayourfriend Feb 28, 2022

dhruvkb Feb 28, 2022 •

edited

Loading

sarayourfriend Feb 28, 2022

dhruvkb Feb 28, 2022

sarayourfriend Feb 28, 2022

sarayourfriend left a comment

sarayourfriend Mar 1, 2022

dhruvkb Mar 1, 2022

sarayourfriend Mar 1, 2022

dhruvkb Mar 1, 2022

dhruvkb Mar 1, 2022

obulat left a comment

		* @param data - the list of data points to interpolate
		* @param threshold - the number of expected data points from the array

		type: process.env.NODE_ENV === 'test' ? Object : String,
		type: isTest ? Object : String,

Convert 6 utils to TypeScript #1023

Convert 6 utils to TypeScript #1023

Conversation

dhruvkb commented Feb 28, 2022 • edited Loading

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

sarayourfriend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruvkb Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruvkb Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruvkb commented Feb 28, 2022

sarayourfriend commented Feb 28, 2022 • edited Loading

sarayourfriend Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruvkb Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruvkb commented Feb 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarayourfriend Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

rbadillap Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruvkb Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarayourfriend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obulat left a comment

Choose a reason for hiding this comment

dhruvkb commented Feb 28, 2022 •

edited

Loading

dhruvkb Feb 28, 2022 •

edited

Loading

dhruvkb Feb 28, 2022 •

edited

Loading

sarayourfriend commented Feb 28, 2022 •

edited

Loading

sarayourfriend Feb 28, 2022 •

edited

Loading

dhruvkb Mar 1, 2022 •

edited

Loading

sarayourfriend Feb 28, 2022 •

edited

Loading

rbadillap Feb 28, 2022 •

edited

Loading

dhruvkb Feb 28, 2022 •

edited

Loading