Slight reorganization around top-of-file comments #84
Conversation
src/utils/get-comment-registry.ts
Outdated
| @@ -243,7 +244,7 @@ export const getCommentRegistryFromImportDeclarations = ({ | |||
| /** Constructed Output Nodes */ | |||
| outputNodes: ImportDeclaration[]; | |||
| }) => { | |||
| if (outputNodes.length === 0 || !firstImport) { | |||
| if (outputNodes?.length === 0 || !firstImport) { | |||
There was a problem hiding this comment.
When would outputNodes be undefined? We have TS to guarantee it's an array just above, right?
There was a problem hiding this comment.
I concur that TypeScript would prevent that.
I wasn’t sure how robust we wanted our src/utils/* to be against misuse.
I wrote a unit test that crashed this, so I made the check verify that it was non-null. All of my Fatal Internal Error checks could be removed if you wanted to do so. I found them helpful during the initial implementation phase.
src/utils/get-comment-registry.ts
Outdated
| @@ -368,7 +369,12 @@ export function attachCommentsToOutputNodes( | |||
| /** Original declaration, not the re-sorted output-node! */ | |||
| firstImport: ImportDeclaration, | |||
| ) { | |||
| if (outputNodes.length === 0) { | |||
| if (!Array.isArray(commentEntriesFromRegistry)) { | |||
There was a problem hiding this comment.
How would this happen without throwing a TypeScript compilation error?
There was a problem hiding this comment.
I concur that TypeScript would prevent that. (Responded further here: #84 (comment))
src/utils/get-comment-registry.ts
Outdated
| 'Fatal Internal Error: Expected a list of commentEntriesFromRegistry', | ||
| ); | ||
| } | ||
| if (outputNodes == null || outputNodes.length === 0) { |
There was a problem hiding this comment.
I concur that TypeScript would prevent that. (Responded further here: #84 (comment))
IanVS
deleted the
fb/protect-the-top-of-file+clarify-patch-first-location-once
branch
I think this makes the code a little easier to follow.
I wasn't able to add an end-to-end integration test around "empty-statements-at-top-of-file" because you're right @IanVS, other layers prevent that from happening.
Instead I added a unit-test to assert the fatal-errors that we throw.