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: upgrade typescript + prettier + eslint with a single shared linting config #424

Merged
merged 18 commits into from
Dec 4, 2024

Conversation

stefl
Copy link
Contributor

@stefl stefl commented Dec 3, 2024

Description

  • Upgrades Typescript to latest
  • Upgrades Eslint to latest
  • Moves all config to a shared eslint-config package
  • Uses the new Eslint config format

We primarily use Typescript, some ESM, and an occasional CommonJS file. So this makes TS the preferred format, ESM the secondary and CommonJS a tertiary format.

To write JS code, name your file with the .mjs extension.
To write code with CommonJS you will need to use the .cjs extension.

Linting rules will now apply to each type of file, and we can specify rules that apply generally, or specifically for certain types of file.

How to review

  • Pull the branch down locally
  • Restart your editor
  • Make changes to a file / break something
  • Make sure that linting is performant and you get feedback as expected
  • Fix the issue
  • Make sure you get feedback quickly that the error is resolved

Copy link

vercel bot commented Dec 3, 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 Dec 3, 2024 8:08pm

"ts-jest": "^29.1.4",
"typescript": "5.3.3",
"ts-jest": "^29.2.5",
"typescript": "5.7.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bumps Typescript to latest

@@ -1,6 +1,6 @@
// Inspired by Chatbot-UI and modified to fit the needs of this project
// @see https://github.com/mckaywrigley/chatbot-ui/blob/main/components/Chat/ChatMessage.tsx
import type { ReactNode } from "react";
import type { ReactNode, JSX } from "react";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Global JSX is now deprecated

@@ -21,7 +21,7 @@ export async function waitForStreamingStatusChange(
await page.waitForFunction(
([currentStatus, expectedStatus]) => {
const statusElement = document.querySelector(
// eslint-disable-next-line @typescript-eslint/quotes, quotes
// eslint-disable-next-line quotes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@typescript-eslint/quotes is deprecated

@@ -9,11 +9,13 @@
"incremental": true,
"esModuleInterop": true,
"module": "esnext",
"moduleResolution": "node",
"moduleResolution": "bundler",
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 back to Bundler throughout

"type-check": "tsc --noEmit"
},
"eslintConfig": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove the per-project config (for now?) and replace with this config in the package.json for each package

},
"engines": {
"node": ">=20.9.0",
"pnpm": ">=8"
}
},
"type": "module"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set all packages as type module

@@ -3,7 +3,7 @@ import { createOpenAIClient } from "@oakai/core/src/llm/openai";
import { aiLogger } from "@oakai/logger";
import fs from "fs/promises";
import type OpenAI from "openai";
import type { ChatCompletionCreateParamsNonStreaming } from "openai/resources";
import type { ChatCompletionCreateParamsNonStreaming } from "openai/resources/index.mjs";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why we have to do this now. This has changed a few times

isSidebarOpen: boolean;
toggleSidebar: () => void;
isLoading: boolean;
}

const SidebarContext = createContext<SidebarContext | undefined>(undefined);
const SidebarContext = createContext<SidebarContextValue | undefined>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The eslint config no longer likes types named the same as code definitions

@@ -0,0 +1,42 @@
{
"name": "@oakai/eslint-config",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes a new package that matches the same naming convention as the prettier config

@@ -0,0 +1,140 @@
import eslint from '@eslint/js';
Copy link
Contributor Author

@stefl stefl Dec 3, 2024

Choose a reason for hiding this comment

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

This is the single source of truth for eslint config. ATM it has all rules for all packages in one file

@@ -0,0 +1,3 @@
declare module 'eslint-plugin-import';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These don't have types

@@ -0,0 +1,64 @@
import tsEslint from '@typescript-eslint/eslint-plugin';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separate out the actual rule definitions

const typescript = tsEslint.configs.recommended;

// @ts-check
/** @type {Partial<Record<string, import('@typescript-eslint/utils/ts-eslint').SharedConfig.RuleEntry>>} */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally had this as a typescript file, but it would have meant having a build step, so this is the next best approach

@@ -78,7 +78,9 @@ export async function lpPartsEmbedStart({
await prisma.ingestLessonPlanPart.updateMany({
where: {
id: {
in: lessonPlanPartIds,
in: lessonPlanPartIds.filter(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes a linting error

"devDependencies": {
"prettier": "^3.1.1"
"@trivago/prettier-plugin-sort-imports": "^4.3.0",
"prettier": "^3.4.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bump prettier to latest

@stefl stefl changed the title fix: upgrade eslint, typescript, move to single shared config (WIP) fix: upgrade typescript + prettier + eslint with a single shared linting config Dec 3, 2024
Copy link

github-actions bot commented Dec 3, 2024

Playwright test results

passed  15 passed
skipped  1 skipped

Details

report  Open report ↗︎
stats  16 tests across 15 suites
duration  2 minutes, 11 seconds
commit  efc69ea

Skipped tests

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

Copy link

sonarqubecloud bot commented Dec 3, 2024

@@ -1,7 +1,7 @@
const github = require("@actions/github");

const prFromSha = require("../pr_from_sha");
const branchFromSha = require("../branch_from_sha");
const prFromSha = require("../pr_from_sha.cjs");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Potentially we could refactor this all to use ESM but this is the minimal change to get this to work with the new requirement to use .cjs extensions for commonjs code

"private": true,
"scripts": {
"build": "next build",
"build:dev": "pnpm with-env next build",
"check": "tsc --noEmit",
"clean": "rm -rf .next .turbo node_modules",
"dev": "FORCE_COLOR=1 SENTRY_SUPPRESS_TURBOPACK_WARNING=1 pnpm with-env node scripts/increase-listeners.js next dev --port 2525 --turbo | pino-pretty -C",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

@@ -1,26 +1,27 @@
{
"name": "@oakai/nextjs",
"version": "0.1.0",
"version": "1.0.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lovely

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.

The code looks great to me. I haven't tested it locally yet

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.

Tested and it seems to work very well with Zed (my editor)

@stefl stefl merged commit 1f4aa9d into main Dec 4, 2024
26 checks passed
@stefl stefl deleted the fix/eslint_ts branch December 4, 2024 18:18
@oak-machine-user
Copy link
Collaborator

🎉 This PR is included in version 1.18.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