-
Notifications
You must be signed in to change notification settings - Fork 264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid circular imports for HMR #2014
Conversation
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
const knownGenerics: Record<string, string | undefined> = { | ||
MetaFunction: 'T', | ||
SerializeFrom: 'T', | ||
}; | ||
|
||
typedefsFromImports.forEach((typeElements, moduleSpecifier) => { | ||
for (const typeElement of typeElements) { | ||
// We only use this in root.tsx and it's better to | ||
// reuse the existing LoaderReturnData, so skip it. | ||
if (typeElement === 'SerializeFrom') continue; | ||
|
||
// Note: SerializeFrom also needs generic if we stop skipping it. | ||
const hasGeneric = !!knownGenerics[typeElement]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ensures that import type {SerializeFrom} '@shopify/remix-oxygen'
is translated correctly to a JSDoc import with a generic (it doesn't work without a generic):
/**
* @template T @typedef {import('@shopify/remix-oxygen').SerializeFrom<T>} SerializeFrom
*/
We do the same for MetaFunction, which also requires a generic.
} else { | ||
// Add return type annotation to plain functions | ||
const declaration = functionNode as FunctionDeclaration; | ||
const jsDocs = declaration.getJsDocs()[0]; | ||
|
||
if (jsDocs?.getTags().length === 0) { | ||
const returnType = declaration.getReturnType().getText(); | ||
if (isAnnotableType(returnType)) { | ||
jsDocs.addTag({tagName: 'return', text: normalizeType(returnType)}); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new part is adding @return
on top of exported functions like export function useRootLoaderData()....
.
The original part was only working for export const useRootLoaderData = () =>...
syntax.
return normalizedType === 'SerializeFrom<any>' | ||
? `{SerializeFrom<loader>}` // Fix inference for loaders outisde of routes | ||
: `{${normalizedType}}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, the library we use (ts-morph
) is inferring SerializeFrom<typeof loader>
as SerializeFrom<any>
. This is a small workaround to get what we want in JSDoc, SerializeFrom<loader>
.
Related #1998
useRootLoaderData
used to work well in the Remix Classic Compiler. However, in Vite it creates issues in development because of circular imports.I'm moving this function to a separate file to avoid that, and ensuring that JSDoc for JS projects still works.
I'm moving it to
lib/root-data.ts
for now but we could also move it tohooks/useRootLoaderData.ts
. We do this in the demo-store but not in skeleton -- we have a hook inlib/variants.ts
so I'm just placing the new one inlib
as well.With these changes we reduce the number of files that need to be refreshed when we change anything in the app. For example, when changing something in
root.tsx
:This is much more noticeable in apps like the demo-store.