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: remaining linting fixes #272

Merged
merged 50 commits into from
Oct 30, 2024
Merged

fix: remaining linting fixes #272

merged 50 commits into from
Oct 30, 2024

Conversation

stefl
Copy link
Contributor

@stefl stefl commented Oct 25, 2024

Description

This is the remaining linting work that has since been merged through other PRs.

  • By being more specific about imports and including types, this aims to reduce how much code has to be processed in builds, potentially reducing test run times and dev server start times, and potentially improve production performance because of smaller bundle sizes

Code conventions:

  • Reintroduces our custom eslint rules with everything set to "warn". Somehow this had got disabled at some point.
  • Adds @typescript-eslint/consistent-type-imports so that we import with import type where appropriate (and VS Code will automatically do this for us)

Aila package:

  • Disables Americanism detection unless you externally initialise it
  • Removes barrel imports and adjust Aila imports to import the appropriate package
  • Fixes some tricky edge cases with the Aila package as a result (failing to await promises when in an error state in streaming, etc.)
  • Allows us to pass in externally-initialised analytics adapters into the package at intialisation, so we don't always have to import all dependencies for every adapter

The touched files count looks high but it's primarily altering import statements throughout

Copy link

vercel bot commented Oct 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
oak-ai-lesson-assistant ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 30, 2024 4:04pm

Copy link

github-actions bot commented Oct 25, 2024

Playwright test results

passed  13 passed
skipped  1 skipped

Details

report  Open report ↗︎
stats  14 tests across 13 suites
duration  1 minute, 52 seconds
commit  8e0b803

Skipped tests

No persona › tests/auth.test.ts › authenticate through Clerk UI

Copy link
Collaborator

@codeincontext codeincontext left a comment

Choose a reason for hiding this comment

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

This is good work

The import changes make sense to me. I don't think they will be fully effective unless we encourage us to continue using the pattern. I think we should remove any export * from ... code so our autosuggest doesn't get tempted to do the wrong thing

The aila changes are really good. I wonder if we will start to need some JSdoc comments on the options so that it's clear what the options are that you can provide when configuring it

Importing inngest at test runtime - not one I'm convinced by at the moment. First off, I don't understand the underlying reason why it would improve performance. Secondly, I don't know if it applies to other packages, other test files, or when we would be able to remove the fix. It would help to see that in a different PR so we can look at a before/ after and get a better understanding of what it's doing and if it's still needed

@stefl
Copy link
Contributor Author

stefl commented Oct 28, 2024

This is good work

The import changes make sense to me. I don't think they will be fully effective unless we encourage us to continue using the pattern. I think we should remove any export * from ... code so our autosuggest doesn't get tempted to do the wrong thing

The aila changes are really good. I wonder if we will start to need some JSdoc comments on the options so that it's clear what the options are that you can provide when configuring it

Importing inngest at test runtime - not one I'm convinced by at the moment. First off, I don't understand the underlying reason why it would improve performance. Secondly, I don't know if it applies to other packages, other test files, or when we would be able to remove the fix. It would help to see that in a different PR so we can look at a before/ after and get a better understanding of what it's doing and if it's still needed

Thanks - I've reverted the inngest import change. A good point about adding jsdoc. Before we go for that, I think we need to look again at the feature factory approach - irrespective of which adapter we use, we are automatically importing all dependencies for all adapters, which feels like something to fix in a separate piece of work. That could change the pattern for how we do this.

@@ -27,6 +27,31 @@ import { router } from "../trpc";

const log = aiLogger("exports");

function getValidLink(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move this function to the body

@stefl stefl changed the title fix: reduce aila import size, use type imports, remove barrel files fix: remaining linting fixes Oct 30, 2024
@stefl stefl requested a review from codeincontext October 30, 2024 16:02
Copy link

@stefl stefl merged commit 18a0f70 into main Oct 30, 2024
17 checks passed
@stefl stefl deleted the fix/reduce_aila_import_size branch October 30, 2024 16:38
codeincontext pushed a commit that referenced this pull request Oct 31, 2024
@oak-machine-user
Copy link
Collaborator

🎉 This PR is included in version 1.14.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants