chore: 🚙 type some helper files and constants #860

tiffafoo · 2021-11-30T17:55:00Z

Summary

Our Netlify Next Plugin is written in mostly JS, but we'd like it to be using TypeScript for readability and prevent some bugs which we've had that could have been avoided with TS. This PR changes a couple files to use TypeScript, as I go through the code and get more familiar with it.

Point of entries

src/helpers/index.js is one of the bigger files, and uses many of the other helpers. Thus I decided to go through the rest of the helpers to minimize impact.

The files touched by this conversion:

src/constants.js
src/helpers/cache.js
src/helpers/config.js

Test plan

Visit the Deploy Preview, see that everything works as expected

Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal

Standard checks:

Check the Deploy Preview's Demo site for your PR's functionality
Add docs when necessary

🧪 Once merged, make sure to update the version if needed and that it was published correctly.

netlify · 2021-11-30T17:55:11Z

✔️ Deploy Preview for netlify-plugin-nextjs-nx-monorepo-demo canceled.

🔨 Explore the source changes: 2c32256

🔍 Inspect the deploy log: https://app.netlify.com/sites/netlify-plugin-nextjs-nx-monorepo-demo/deploys/61a74200020e9200077bfc6d

tiffafoo · 2021-11-30T17:56:20Z

src/helpers/config.ts

@@ -123,6 +131,8 @@ exports.generateRedirects = async ({ netlifyConfig, basePath, i18n }) => {
      from: `${basePath}/*`,
      to: HANDLER_FUNCTION_PATH,
      status: 200,
+      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+      // @ts-ignore


I left that in as I did the first pass to TS. I figured we'd want to get this in incrementally and worry too much about problematic missing types for now.

Is this type missing in @netlify/build then?

Nah just one that would require a bit of refactoring of the function this ts-ignore refers to

tiffafoo · 2021-11-30T17:56:42Z

src/helpers/config.ts

    if (!config) {
+      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+      // @ts-ignore


Same here, expects a second object and I just ignored it as I passed through :P

tiffafoo · 2021-11-30T17:57:07Z

src/helpers/config.ts

      return failBuild('Error loading your Next config')
    }
    return { ...config, appDir, ignore }
-  } catch (error) {
+  } catch (error: unknown) {


It is best practice to have errors as unknown since there's no good way to predict the type of an error

ascorbic

Fantastic! Thanks so much for doign this.

ascorbic · 2021-11-30T20:39:25Z

src/helpers/config.ts

+import { NetlifyConfig } from '@netlify/build'
+import { yellowBright } from 'chalk'
+import { readJSON } from 'fs-extra'
+import { PrerenderManifest } from 'next/dist/build'


Can this be import type?

I can't do import { PrerenderManifest } from 'next/types' since they don't have it in their types, is that what you mean?

No, I just mean use the type import syntax to make it clear that you're only importing the type:

import type { PrerenderManifest } from 'next/dist/build'

ascorbic · 2021-11-30T20:40:34Z

src/helpers/config.ts

+export const generateRedirects = async ({ netlifyConfig, basePath, i18n }: { 
+  netlifyConfig: NetlifyConfig, 
+  basePath: string, 
+  i18n 


You can get this type as NextConfig["i18n"] from next

oo thanks! Wasn't sure where it was coming from

ascorbic · 2021-11-30T20:43:33Z

src/helpers/config.ts

@@ -123,6 +131,8 @@ exports.generateRedirects = async ({ netlifyConfig, basePath, i18n }) => {
      from: `${basePath}/*`,
      to: HANDLER_FUNCTION_PATH,
      status: 200,
+      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+      // @ts-ignore


Is this type missing in @netlify/build then?

ascorbic · 2021-11-30T20:46:18Z

src/helpers/config.ts

@@ -180,7 +192,7 @@ const resolveModuleRoot = (moduleName) => {

 const DEFAULT_EXCLUDED_MODULES = ['sharp', 'electron']

-exports.configureHandlerFunctions = ({ netlifyConfig, publish, ignore = [] }) => {
+export const configureHandlerFunctions = ({ netlifyConfig, publish, ignore = [] }) => {


Can these args be typed?

ascorbic · 2021-11-30T20:47:30Z

src/helpers/requiredServerFilesType.ts

@@ -0,0 +1,129 @@
+/* eslint-disable eslint-comments/disable-enable-pair, @typescript-eslint/no-empty-interface, no-use-before-define */


Ah, nice. Maybe we should contribute this upstream!

Good call! Although I'm sure the definition file is missing some types since it was just auto-generated based on the demo's required-server-files.json

ascorbic · 2021-11-30T20:50:06Z

src/helpers/cache.ts

-} = require('path')
+import { posix } from 'path'
+
+export const restoreCache = async ({ cache, publish }) => {


I htink it's a good idea if we ensure that all exported functions have typed args and return values, even if they're just defined inline.

ascorbic · 2021-11-30T20:53:12Z

test/__snapshots__/index.js.snap

-  "fr/static.html",
-]
-`;
+exports[`onBuild() generates static files manifest 1`] = `Array []`;


You updated the snapshot after running ntl build, which means the files had already been moved. This is why I added the updateSnapshot npm script: you need to just build the site, not run the plugin too.

ascorbic

snapidoo!

tiffafoo · 2021-12-01T09:39:18Z

src/helpers/requiredServerFilesType.ts

+export interface Config {
+    env?:                         Env;
+    webpack?:                     null;
+    webpackDevMiddleware?:        null;


^ these will have to be manually updated, since it's clearly wrong

Tiffany Le-Nguyen added 2 commits November 30, 2021 20:46

chore: type a couple files

3d1ec51

Merge branch 'main' into tln/type-all-the-things-1

c6e4e29

github-actions bot added the type: chore work needed to keep the product and development running smoothly label Nov 30, 2021

tiffafoo commented Nov 30, 2021

View reviewed changes

chore: update snapidoos

8ef023c

ascorbic previously approved these changes Nov 30, 2021

View reviewed changes

ascorbic reviewed Nov 30, 2021

View reviewed changes

chore: revert snapidoo

2c32256

tiffafoo dismissed ascorbic’s stale review via 2c32256 December 1, 2021 09:35

tiffafoo added the automerge label Dec 1, 2021

ascorbic approved these changes Dec 1, 2021

View reviewed changes

kodiakhq bot merged commit d49f56a into main Dec 1, 2021

kodiakhq bot deleted the tln/type-all-the-things-1 branch December 1, 2021 09:39

tiffafoo commented Dec 1, 2021

View reviewed changes

		@@ -0,0 +1,129 @@
		/* eslint-disable eslint-comments/disable-enable-pair, @typescript-eslint/no-empty-interface, no-use-before-define */

chore: 🚙 type some helper files and constants #860

chore: 🚙 type some helper files and constants #860

Uh oh!

Conversation

tiffafoo commented Nov 30, 2021

Summary

Point of entries

Test plan

Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal

Standard checks:

Uh oh!

netlify bot commented Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ascorbic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ascorbic Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ascorbic Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ascorbic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

netlify bot commented Nov 30, 2021 •

edited

Loading

ascorbic Nov 30, 2021 •

edited

Loading

ascorbic Nov 30, 2021 •

edited

Loading