From 2699c603f05647c0382c7f7f84109a2a940dd3fb Mon Sep 17 00:00:00 2001 From: David Crespo Date: Sun, 12 Sep 2021 16:13:03 -0500 Subject: [PATCH 1/7] invariant assert helper --- app/util/invariant.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 app/util/invariant.ts diff --git a/app/util/invariant.ts b/app/util/invariant.ts new file mode 100644 index 0000000000..2d548cf4eb --- /dev/null +++ b/app/util/invariant.ts @@ -0,0 +1,15 @@ +// Used to assert runtime invariants in development and test. Miraculously, +// Rollup not only minifies out the inside of the function when `NODE_ENV` is +// 'production', it actually removes the entire function call from the calling +// code, which means arbitrarily long error messages don't end up in the +// production bundle. This was manually tested in the production build to make +// sure, but here's an example in Rollup REPL: https://bit.ly/3z5cbQG + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +export function invariant(condition: any, message: string) { + if (process.env.NODE_ENV !== 'production') { + if (!condition) { + throw new Error(message) + } + } +} From be7163ff8e30d1a1f46d4fc07c27698b8fc5695c Mon Sep 17 00:00:00 2001 From: David Crespo Date: Sun, 12 Sep 2021 22:01:24 -0500 Subject: [PATCH 2/7] jsdoc comment format so VS Code picks it up --- app/util/invariant.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/app/util/invariant.ts b/app/util/invariant.ts index 2d548cf4eb..d99a1ee1b2 100644 --- a/app/util/invariant.ts +++ b/app/util/invariant.ts @@ -1,10 +1,7 @@ -// Used to assert runtime invariants in development and test. Miraculously, -// Rollup not only minifies out the inside of the function when `NODE_ENV` is -// 'production', it actually removes the entire function call from the calling -// code, which means arbitrarily long error messages don't end up in the -// production bundle. This was manually tested in the production build to make -// sure, but here's an example in Rollup REPL: https://bit.ly/3z5cbQG - +/** + * Throw with mesasage if condition is falsy. Entire call stripped out by + * Rollup in prod. + */ // eslint-disable-next-line @typescript-eslint/no-explicit-any export function invariant(condition: any, message: string) { if (process.env.NODE_ENV !== 'production') { @@ -13,3 +10,9 @@ export function invariant(condition: any, message: string) { } } } + +// Miraculously, Rollup not only minifies out the inside of the function when +// `NODE_ENV` is 'production', it actually removes the entire function call from +// the calling code, which means long error messages don't end up in the +// production bundle. This was manually tested in the production build to make +// sure, but here's an example in Rollup REPL: https://bit.ly/3z5cbQG From 74f4d4903ef437afae97427d96f84db7e5b017bb Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 10 Sep 2021 23:47:30 -0500 Subject: [PATCH 3/7] elaborate route params invariant asserter --- app/components/InstancePageHeader.tsx | 5 +++-- app/hooks/index.ts | 5 +++-- app/hooks/use-params.ts | 21 +++++++++++++++++++++ 3 files changed, 27 insertions(+), 4 deletions(-) create mode 100644 app/hooks/use-params.ts diff --git a/app/components/InstancePageHeader.tsx b/app/components/InstancePageHeader.tsx index 6d62567673..a8904cf9b6 100644 --- a/app/components/InstancePageHeader.tsx +++ b/app/components/InstancePageHeader.tsx @@ -1,10 +1,11 @@ import React from 'react' -import { useParams, useNavigate } from 'react-router-dom' +import { useNavigate } from 'react-router-dom' import { Menu, MenuList, MenuButton, MenuItem } from '@reach/menu-button' import { instanceCan, useApiQuery, useApiMutation } from '@oxide/api' import { Icon, PageHeader, PageTitle } from '@oxide/ui' +import { useParams } from '../hooks' import { InstanceDetails } from './InstanceDetails' import { useToast } from '../hooks' @@ -12,7 +13,7 @@ import { useToast } from '../hooks' export const InstancePageHeader = () => { const navigate = useNavigate() const addToast = useToast() - const { projectName, instanceName } = useParams() + const { projectName, instanceName } = useParams('projectName', 'instanceName') const { data: instance, diff --git a/app/hooks/index.ts b/app/hooks/index.ts index 9494eb4d1e..82c25a91d8 100644 --- a/app/hooks/index.ts +++ b/app/hooks/index.ts @@ -1,3 +1,4 @@ -export * from './use-toast' -export * from './use-pagination' export * from './use-key' +export * from './use-pagination' +export * from './use-params' +export * from './use-toast' diff --git a/app/hooks/use-params.ts b/app/hooks/use-params.ts new file mode 100644 index 0000000000..161ee4e065 --- /dev/null +++ b/app/hooks/use-params.ts @@ -0,0 +1,21 @@ +import { useParams as _useParams } from 'react-router' +import { invariant } from '../util/invariant' + +/** + * Wrapper for React Router's `useParams` that throws (in dev) if any of the + * specified params is missing. + * + * @returns an object where the params are guaranteed (in dev) to be present + */ +export function useParams( + ...paramNames: K[] +): Record { + const params = _useParams() + for (const k of paramNames) { + invariant( + k in params, + `Param '${k}' not found in route. You might be rendering a component under the wrong route.` + ) + } + return params as Record +} From 782ca06c3339deb7b737122390d57f282bc8d634 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 14 Oct 2021 10:23:35 -0500 Subject: [PATCH 4/7] fix typo --- app/util/invariant.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/util/invariant.ts b/app/util/invariant.ts index d99a1ee1b2..f865305b8c 100644 --- a/app/util/invariant.ts +++ b/app/util/invariant.ts @@ -1,5 +1,5 @@ /** - * Throw with mesasage if condition is falsy. Entire call stripped out by + * Throw with message if condition is falsy. Entire call stripped out by * Rollup in prod. */ // eslint-disable-next-line @typescript-eslint/no-explicit-any From f3a209ce26ef62c93d77ed8df95d2d596e5fad4a Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 14 Oct 2021 21:28:13 -0500 Subject: [PATCH 5/7] wrap loop in its own production check --- app/hooks/use-params.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/app/hooks/use-params.ts b/app/hooks/use-params.ts index 161ee4e065..5a6aaf49a6 100644 --- a/app/hooks/use-params.ts +++ b/app/hooks/use-params.ts @@ -11,11 +11,13 @@ export function useParams( ...paramNames: K[] ): Record { const params = _useParams() - for (const k of paramNames) { - invariant( - k in params, - `Param '${k}' not found in route. You might be rendering a component under the wrong route.` - ) + if (process.env.NODE_ENV !== 'production') { + for (const k of paramNames) { + invariant( + k in params, + `Param '${k}' not found in route. You might be rendering a component under the wrong route.` + ) + } } return params as Record } From d8c22cf5f1243a04bca0395c7c88cf6712c1b3ca Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 14 Oct 2021 21:45:11 -0500 Subject: [PATCH 6/7] convert all useParams to our useParams --- app/components/InstancePageHeader.tsx | 4 +--- app/components/InstancesTable.tsx | 10 +++++----- app/hooks/use-params.ts | 4 +++- app/layouts/InstanceLayout.tsx | 5 +++-- app/layouts/ProjectLayout.tsx | 5 +++-- app/pages/instances/Storage.tsx | 4 ++-- app/pages/instances/create/existing-disk-modal.tsx | 4 ++-- app/pages/instances/create/index.tsx | 5 +++-- app/pages/project/Storage.tsx | 4 ++-- app/pages/project/index.tsx | 5 +++-- 10 files changed, 27 insertions(+), 23 deletions(-) diff --git a/app/components/InstancePageHeader.tsx b/app/components/InstancePageHeader.tsx index a8904cf9b6..a6299f935a 100644 --- a/app/components/InstancePageHeader.tsx +++ b/app/components/InstancePageHeader.tsx @@ -3,12 +3,10 @@ import { useNavigate } from 'react-router-dom' import { Menu, MenuList, MenuButton, MenuItem } from '@reach/menu-button' import { instanceCan, useApiQuery, useApiMutation } from '@oxide/api' - import { Icon, PageHeader, PageTitle } from '@oxide/ui' -import { useParams } from '../hooks' +import { useParams, useToast } from '../hooks' import { InstanceDetails } from './InstanceDetails' -import { useToast } from '../hooks' export const InstancePageHeader = () => { const navigate = useNavigate() diff --git a/app/components/InstancesTable.tsx b/app/components/InstancesTable.tsx index c07e12ed52..0152e5a992 100644 --- a/app/components/InstancesTable.tsx +++ b/app/components/InstancesTable.tsx @@ -1,5 +1,5 @@ import React from 'react' -import { useParams, Link } from 'react-router-dom' +import { Link } from 'react-router-dom' import type { Row } from 'react-table' import { useTable, useRowSelect } from 'react-table' import { Menu, MenuList, MenuButton, MenuItem } from '@reach/menu-button' @@ -15,7 +15,7 @@ import { import { classed, Icon, selectCol, Table } from '@oxide/ui' import { StatusBadge } from './StatusBadge' import { timeAgoAbbr } from '../util/date' -import { usePagination, useToast } from '../hooks' +import { usePagination, useParams, useToast } from '../hooks' const columns = [ { @@ -24,7 +24,7 @@ const columns = [ Cell: ({ value }: { value: string }) => { // TODO: is it weird to pull directly from params here and in the menu // column? seems easier than passing it in somehow - const { projectName } = useParams() + const { projectName } = useParams('projectName') return ( { const { currentPage, goToNextPage, goToPrevPage, hasPrev } = usePagination() - const { projectName } = useParams() + const { projectName } = useParams('projectName') const { data: instances } = useApiQuery( 'projectInstancesGet', { projectName, pageToken: currentPage, limit: PAGE_SIZE }, diff --git a/app/hooks/use-params.ts b/app/hooks/use-params.ts index 5a6aaf49a6..947cc27c3a 100644 --- a/app/hooks/use-params.ts +++ b/app/hooks/use-params.ts @@ -11,7 +11,9 @@ export function useParams( ...paramNames: K[] ): Record { const params = _useParams() - if (process.env.NODE_ENV !== 'production') { + // exclude both test and production + // TODO: don't exclude test, that seems bad? + if (process.env.NODE_ENV === 'development') { for (const k of paramNames) { invariant( k in params, diff --git a/app/layouts/InstanceLayout.tsx b/app/layouts/InstanceLayout.tsx index b0de7ce597..c52d9bf2dc 100644 --- a/app/layouts/InstanceLayout.tsx +++ b/app/layouts/InstanceLayout.tsx @@ -1,5 +1,5 @@ import React from 'react' -import { Outlet, useParams } from 'react-router-dom' +import { Outlet } from 'react-router-dom' import { Icon, SkipLinkTarget } from '@oxide/ui' import { @@ -13,9 +13,10 @@ import { } from './helpers' import { Breadcrumbs } from '../components/Breadcrumbs' import { InstancePageHeader } from '../components/InstancePageHeader' +import { useParams } from '../hooks' const InstanceLayout = () => { - const { instanceName } = useParams() + const { instanceName } = useParams('instanceName') return ( diff --git a/app/layouts/ProjectLayout.tsx b/app/layouts/ProjectLayout.tsx index ead86df0bb..fb8c85ebab 100644 --- a/app/layouts/ProjectLayout.tsx +++ b/app/layouts/ProjectLayout.tsx @@ -1,5 +1,5 @@ import React from 'react' -import { Outlet, useParams } from 'react-router-dom' +import { Outlet } from 'react-router-dom' import { Icon, SkipLinkTarget } from '@oxide/ui' import { @@ -12,9 +12,10 @@ import { SidebarDivider, } from './helpers' import { Breadcrumbs } from '../components/Breadcrumbs' +import { useParams } from '../hooks' const ProjectLayout = () => { - const { projectName } = useParams() + const { projectName } = useParams('projectName') return ( diff --git a/app/pages/instances/Storage.tsx b/app/pages/instances/Storage.tsx index ee03bdc9a3..5f3552654c 100644 --- a/app/pages/instances/Storage.tsx +++ b/app/pages/instances/Storage.tsx @@ -1,10 +1,10 @@ import React from 'react' -import { useParams } from 'react-router' import { useTable } from 'react-table' import type { DiskAttachment } from '@oxide/api' import { useApiQuery } from '@oxide/api' import { Button, Table } from '@oxide/ui' +import { useParams } from '../../hooks' const columns = [ { @@ -36,7 +36,7 @@ const columns = [ ] function Storage() { - const { projectName, instanceName } = useParams() + const { projectName, instanceName } = useParams('projectName', 'instanceName') const { data } = useApiQuery( 'instanceDisksGet', { projectName, instanceName }, diff --git a/app/pages/instances/create/existing-disk-modal.tsx b/app/pages/instances/create/existing-disk-modal.tsx index 35a5b3c877..fc13876f81 100644 --- a/app/pages/instances/create/existing-disk-modal.tsx +++ b/app/pages/instances/create/existing-disk-modal.tsx @@ -1,11 +1,11 @@ import React from 'react' -import { useParams } from 'react-router' import { Dialog } from '@reach/dialog' import { Formik, Form } from 'formik' import type { Disk } from '@oxide/api' import { useApiQuery } from '@oxide/api' import { Button, Dropdown, Icon, Radio, RadioGroup } from '@oxide/ui' +import { useParams } from '../../../hooks' const headingStyle = 'font-medium mt-6 mb-3' @@ -24,7 +24,7 @@ const isUnattached = ({ state }: Disk) => { } export function ExistingDiskModal({ isOpen, onDismiss }: Props) { - const { projectName } = useParams() + const { projectName } = useParams('projectName') const { data } = useApiQuery('projectDisksGet', { projectName }) const disks = (data?.items || []) .filter(isUnattached) diff --git a/app/pages/instances/create/index.tsx b/app/pages/instances/create/index.tsx index 054a77e31a..083c281f42 100644 --- a/app/pages/instances/create/index.tsx +++ b/app/pages/instances/create/index.tsx @@ -1,5 +1,5 @@ import React, { useState } from 'react' -import { useParams, useNavigate } from 'react-router-dom' +import { useNavigate } from 'react-router-dom' import { Tab, TabList, TabPanel, TabPanels, Tabs } from '@reach/tabs' import cn from 'classnames' import { Formik, Form } from 'formik' @@ -24,6 +24,7 @@ import { INSTANCE_SIZES } from './instance-types' import { NewDiskModal } from './new-disk-modal' import { ExistingDiskModal } from './existing-disk-modal' import { NetworkModal } from './network-modal' +import { useParams } from '../../../hooks' // TODO: these probably should not both exist const headingStyle = 'text-white text-display-xl font-sans font-light' @@ -310,7 +311,7 @@ export function InstanceCreateForm({ projectName }: { projectName: string }) { } const InstanceCreatePage = () => { - const { projectName } = useParams() + const { projectName } = useParams('projectName') return ( <> diff --git a/app/pages/project/Storage.tsx b/app/pages/project/Storage.tsx index e8ee46d255..c2482c6825 100644 --- a/app/pages/project/Storage.tsx +++ b/app/pages/project/Storage.tsx @@ -1,9 +1,9 @@ import React from 'react' -import { useParams } from 'react-router' import { useTable } from 'react-table' import { useApiQuery } from '@oxide/api' import { Table } from '@oxide/ui' +import { useParams } from '../../hooks' const columns = [ { @@ -19,7 +19,7 @@ const columns = [ ] export default function ProjectStorage() { - const { projectName } = useParams() + const { projectName } = useParams('projectName') const { data } = useApiQuery( 'projectDisksGet', { projectName }, diff --git a/app/pages/project/index.tsx b/app/pages/project/index.tsx index f710974dbd..4d368fc5d0 100644 --- a/app/pages/project/index.tsx +++ b/app/pages/project/index.tsx @@ -1,12 +1,13 @@ import React from 'react' -import { useParams, Link } from 'react-router-dom' +import { Link } from 'react-router-dom' import { useApiQuery } from '@oxide/api' import { buttonStyle, PageHeader, PageTitle } from '@oxide/ui' import { InstancesTable } from '../../components/InstancesTable' +import { useParams } from '../../hooks' const ProjectPage = () => { - const { projectName } = useParams() + const { projectName } = useParams('projectName') const { data: project } = useApiQuery('projectsGetProject', { projectName, }) From 865c8a7fc295b2c839a112ef50097cf0b9879f07 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 15 Oct 2021 11:12:00 -0500 Subject: [PATCH 7/7] run params runtime check in test --- app/hooks/use-params.ts | 4 +--- .../__tests__/InstanceCreateForm.spec.tsx | 19 +++++++++++-------- .../instances/create/existing-disk-modal.tsx | 6 +++--- app/pages/instances/create/index.tsx | 1 + 4 files changed, 16 insertions(+), 14 deletions(-) diff --git a/app/hooks/use-params.ts b/app/hooks/use-params.ts index 947cc27c3a..5a6aaf49a6 100644 --- a/app/hooks/use-params.ts +++ b/app/hooks/use-params.ts @@ -11,9 +11,7 @@ export function useParams( ...paramNames: K[] ): Record { const params = _useParams() - // exclude both test and production - // TODO: don't exclude test, that seems bad? - if (process.env.NODE_ENV === 'development') { + if (process.env.NODE_ENV !== 'production') { for (const k of paramNames) { invariant( k in params, diff --git a/app/pages/__tests__/InstanceCreateForm.spec.tsx b/app/pages/__tests__/InstanceCreateForm.spec.tsx index 51ad67eac1..8021f8d836 100644 --- a/app/pages/__tests__/InstanceCreateForm.spec.tsx +++ b/app/pages/__tests__/InstanceCreateForm.spec.tsx @@ -15,10 +15,13 @@ import { InstanceCreateForm } from '../instances/create' const submitButton = () => screen.getByRole('button', { name: 'Create instance' }) -const postUrl = `/api/projects/${project.name}/instances` +const instancesUrl = `/api/projects/${project.name}/instances` +const disksUrl = `/api/projects/${project.name}/disks` describe('InstanceCreateForm', () => { beforeEach(() => { + // existing disk modal fetches disks on render even if it's not visible + fetchMock.get(disksUrl, 200) renderWithRouter() }) @@ -27,21 +30,21 @@ describe('InstanceCreateForm', () => { }) it('disables submit button on submit and enables on response', async () => { - const mock = fetchMock.post(postUrl, { status: 201 }) + const mock = fetchMock.post(instancesUrl, 201) const submit = submitButton() expect(submit).not.toBeDisabled() fireEvent.click(submit) - expect(mock.called()).toBeFalsy() + expect(mock.called(instancesUrl)).toBeFalsy() await waitFor(() => expect(submit).toBeDisabled()) expect(mock.done()).toBeTruthy() expect(submit).not.toBeDisabled() }) it('shows specific message for known server error code', async () => { - fetchMock.post(postUrl, { + fetchMock.post(instancesUrl, { status: 400, body: { error_code: 'ObjectAlreadyExists' }, }) @@ -54,7 +57,7 @@ describe('InstanceCreateForm', () => { }) it('shows generic message for unknown server error', async () => { - fetchMock.post(postUrl, { status: 400 }) + fetchMock.post(instancesUrl, 400) fireEvent.click(submitButton()) @@ -62,7 +65,7 @@ describe('InstanceCreateForm', () => { }) it('posts form on submit', async () => { - const mock = fetchMock.post(postUrl, { status: 201 }) + const mock = fetchMock.post(instancesUrl, 201) fireEvent.change(screen.getByLabelText('Choose a name'), { target: { value: 'new-instance' }, @@ -82,14 +85,14 @@ describe('InstanceCreateForm', () => { }) it('navigates to project page on success', async () => { - const mock = fetchMock.post(postUrl, { status: 201, body: instance }) + const mock = fetchMock.post(instancesUrl, { status: 201, body: instance }) const projectPath = `/projects/${project.name}` expect(window.location.pathname).not.toEqual(projectPath) fireEvent.click(submitButton()) - await waitFor(() => expect(mock.called()).toBeTruthy()) + await waitFor(() => expect(mock.called(instancesUrl)).toBeTruthy()) await waitFor(() => expect(mock.done()).toBeTruthy()) await waitFor(() => expect(window.location.pathname).toEqual(projectPath)) }) diff --git a/app/pages/instances/create/existing-disk-modal.tsx b/app/pages/instances/create/existing-disk-modal.tsx index fc13876f81..d0cd23a8e7 100644 --- a/app/pages/instances/create/existing-disk-modal.tsx +++ b/app/pages/instances/create/existing-disk-modal.tsx @@ -5,13 +5,13 @@ import { Formik, Form } from 'formik' import type { Disk } from '@oxide/api' import { useApiQuery } from '@oxide/api' import { Button, Dropdown, Icon, Radio, RadioGroup } from '@oxide/ui' -import { useParams } from '../../../hooks' const headingStyle = 'font-medium mt-6 mb-3' type Props = { isOpen: boolean onDismiss: () => void + projectName: string } const isUnattached = ({ state }: Disk) => { @@ -23,8 +23,8 @@ const isUnattached = ({ state }: Disk) => { ) } -export function ExistingDiskModal({ isOpen, onDismiss }: Props) { - const { projectName } = useParams('projectName') +export function ExistingDiskModal({ isOpen, onDismiss, projectName }: Props) { + // TODO: maybe wait to fetch until you open the modal const { data } = useApiQuery('projectDisksGet', { projectName }) const disks = (data?.items || []) .filter(isUnattached) diff --git a/app/pages/instances/create/index.tsx b/app/pages/instances/create/index.tsx index 083c281f42..d40b946a17 100644 --- a/app/pages/instances/create/index.tsx +++ b/app/pages/instances/create/index.tsx @@ -238,6 +238,7 @@ export function InstanceCreateForm({ projectName }: { projectName: string }) { setShowExistingDiskModal(false)} + projectName={projectName} />