Skip to content
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

Fix tsconfig.json and Lint errors #290

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

derekvmcintire
Copy link
Contributor

@derekvmcintire derekvmcintire commented Jan 9, 2025

Summary of changes

  • Updated tsconfig.json - left comments on each line to document the purpose of each configuration
  • Fixed type and lint errors where I could find them including:
    • import order
    • file extensions in imports
    • vitest methods
    • explicit type imports
    • typecasting where necessary
    • declared a type module for heat-stack/types/vite-env-only.d.ts - not sure if there is a better solution for this, but it works

Unfixable Type Errors

There were unfixable errors in heat-stack/app/utils/providers/github.server.ts which i typecasted to unknown before typecasting again to the needed type, but I suspect there could be issues with either the data or the type here that need to be addressed further. I left @TODOs in the code - but it might be better to create an issue if there isn't a straight forward answer here.

Lint Errors Not Fixed

There are still some lint errors from a React hook being used in a test outside of a component (it is possible the use() method here is intended to be imported from somewhere else?)

Screenshots

Screenshot 2025-01-09 at 4 50 35 PM
Screenshot 2025-01-09 at 4 51 31 PM

return new GitHubStrategy(
{
clientID: process.env.GITHUB_CLIENT_ID,
clientSecret: process.env.GITHUB_CLIENT_SECRET,
callbackURL: '/auth/github/callback',
},
} as unknown as GitHubStrategyOptions, // @TODO fix types here, clientID and callbackURL do not exist on GitHubStrategyOptions
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@todo here because clientID and callbackURL do not exist on type GitHubStrategyOptions, so we either need to figure out what type we should be using, or potentially update the code here

@@ -25,20 +26,20 @@ const shouldMock =
process.env.NODE_ENV === 'test'

export class GitHubProvider implements AuthProvider {
getAuthStrategy() {
getAuthStrategy(): Strategy<ProviderUser, any> { // @TODO double check the types here
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added @todo here for someone to double check the types

@derekvmcintire
Copy link
Contributor Author

Updates 1/15/2025

This should be working now. Here is what I discovered:

  1. The giant list of errors was from the new changes that had just been merged in (Thad, you might have pointed this out but it didn't sink in last night). So I fetched and merged the new changes and was able to reproduce locally.
  2. The new typescript configuration requires explicit file extensions in imports, and they were missing from the new changes. I fixed them, and then re-configured my VSCode to add file extensions when using auto imports (details below).
  3. The error we were seeing from the ../build/... directory was not caused by typescript looking at the build directory itself, but it is actually a dynamic import in server/index.tsx: await import('../build/server/index.js'). There was already a tsignore comment above it: // @ts-expect-error - the file might not exist yet but it will but I had accidentally added a space between @ and t so it wasn't working.

More Details

To configure VSCode to add file extensions automatically

  1. Go to your settings.json file: cmd + shift + p, then select “Open User Settings (JSON)
  2. Add the following (or replace existing configurations):
"typescript.preferences.importModuleSpecifier": "relative",  // or "auto" for dynamic imports
"typescript.preferences.importModuleSpecifierEnding": "minimal",  // "auto" if possible

Going forward, you will see typescript errors for imports without file extensions

This is because of our "module": "NodeNext" and "moduleResolution": "nodenext" settings in tsconfig.json. Below is a quick overview of why we probably want this setting, but if we feel like this is something we don't want to do I can try to work out a different ts configuration.

  • Requiring file extensions:
  • Pros:
    • Aligns with the current Node.js and ES Module specification, and makes our project more compatible with modern JavaScript standards.
    • Helps avoid ambiguity when resolving files, especially when working across different platforms.
    • More consistent and predictable behavior, especially when moving code between environments.
  • Cons:
    • Requires us to be more explicit with imports.
    • It’s an extra thing to manage.

Not requiring file extensions (i.e., trying to configure TypeScript to automatically infer extensions):

  • Pros:
    • The import syntax feels cleaner and more concise.
    • Matches the behavior we might be accustomed to in other TypeScript or JavaScript projects.
  • Cons:
    • Potential compatibility issues with certain tools or libraries that rely on explicit file extensions.
    • Can lead to bugs when files are not resolved correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant