Skip to content

Update TypeScript and ESLint configs for compatibility #5

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

Merged
merged 4 commits into from
Apr 5, 2023
Merged

Update TypeScript and ESLint configs for compatibility #5

merged 4 commits into from
Apr 5, 2023

Conversation

mcous
Copy link
Contributor

@mcous mcous commented Apr 4, 2023

Overview

This PR makes some changes in reaction to integrating these configs into viamrobotics/prime.

  1. Change how @viamrobotics/typescript-config specifies exports
  2. Fix some ESLint rules in reaction to how we actually write code
  3. Prevent internal tsconfig.json files from being published in the modules

I tested these changes locally with Prime.

Changelog

TypeScript

Lesson no. 1 from integrating the common configs into a project: the TypeScript config world is not yet ready for packageJson.exports. Support in tsc itself is barely there in v5 (e.g. see microsoft/TypeScript#53314), and other related tooling - like get-tsconfig, used in eslint-plugin-import - completely chokes on it.

This PR updates the TypeScript config package to remove exports, so package consumers can specify file paths directly, and updates the documentation. It's less neat, but more pragmatic.

{
  // before
  "extends": "@viamrobotics/typescript-config"
  
  // after
  "extends": "@viamrobotics/typescript-config/base.json"
}

ESLint

  • Remove no-duplicate-imports, it's redundant with the already enabled import/no-duplicates and is annoying about type imports
  • Replace no-use-before-define with type-aware @typescript-eslint/no-use-before-define and configure to only catch actual errors
    • The default configuration triggers annoying false positives for stuff like...
    const handleEvent = () => {
      cleanup()  // <- works because cleanup is in a higher scope, but no-use-before-define complains
      console.log('event!')
    }
    
    const cleanup = () => {
      emitter.off('event', handleEvent)
    }
    
    emitter.on('event', handleEvent)

@mcous mcous requested review from DTCurrie and micheal-parks April 4, 2023 20:08
@mcous mcous changed the title Do not use exports field for TS config Update TypeScript config and ESLint config for compatibility Apr 4, 2023
@mcous mcous changed the title Update TypeScript config and ESLint config for compatibility Update TypeScript and ESLint configs for compatibility Apr 5, 2023
Copy link
Member

@DTCurrie DTCurrie left a comment

Choose a reason for hiding this comment

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

@mcous mcous merged commit 494ce9e into viamrobotics:main Apr 5, 2023
@mcous mcous deleted the typescript-no-exports branch April 5, 2023 20:11
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.

2 participants