code review suggestions #45
Conversation
src/utilities/currentLocation.ts
Outdated
There was a problem hiding this comment.
Unless we're importing getInitialUrl and testing that specifically this is still testing createRouter. So I wouldn't rename these test files.
| import { createRouter } from '@/utilities/createRouter' | ||
| import { component } from '@/utilities/testHelpers' | ||
|
|
||
| describe('getInitialRoute', () => { |
There was a problem hiding this comment.
Do you think every test should be wrapped in a describe? Vitest automatically groups tests by their file so IMO wrapping all the tests in a describe with the name of the utility being tested (which is the same as the name of the file) is not needed and we should reserve describe for organizing larger test files where we want to group tests of similar nature. If every test is in a single describe we don't need it IMO.
src/utilities/getInitialRoute.ts
Outdated
There was a problem hiding this comment.
Abstracting this means less code in createRouter so I'm fine with this. But I don't think an isBrowser existing in a function means it much be abstracted.
|
|
||
| test('defaults to window.location', () => { | ||
| const initialRoute = random.number().toString() | ||
| vi.stubGlobal('location', initialRoute) |
There was a problem hiding this comment.
Nice, TIL. This will be helpful.
No description provided.