- 
                Notifications
    You must be signed in to change notification settings 
- Fork 8
chore: refactor tsconfigs in packages to reuse "include", "exclude" and "outDir" #582
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
Conversation
| "bootstrap": "npm run compile", | ||
| "prepublishOnly": "npm run compile", | ||
| "compile": "tsc -p tsconfig.json && gen-esm-wrapper . ./.esm-wrapper.mjs && prettier --write .esm-wrapper.mjs", | ||
| "compile": "tsc -p tsconfig.json && gen-esm-wrapper . ./dist/.esm-wrapper.mjs", | 
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.
Moving the esm-wrapper into the dist directory like every other package.
4a6210d    to
    8ff1a78      
    Compare
  
    8ff1a78    to
    e983925      
    Compare
  
    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.
Seems like a reasonable way to align packages more 🙂
| @@ -1,15 +1,9 @@ | |||
| { | |||
| "extends": "@mongodb-js/tsconfig-devtools/tsconfig.common.json", | |||
| "extends": "@mongodb-js/tsconfig-devtools/tsconfig.react.json", | |||
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.
There's no react in devtools-connect anymore, right? If you're referring to the "jsx": "react" bit below, I think you can just remove that at this point
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.
I was referring to the "jsx": "react" yes. Reverted this change and it still builds 👍
| "strict": true | ||
| }, | ||
| "include": ["src/**/*"], | ||
| "exclude": ["./src/types/*"], | 
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 exclude isn't kept, right?
I'm a bit confused though, I don't think this worked previously as intended, with two keys of the same name in this file
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.
Yeah - it never worked 🙃 Just tested by reverting this change and tsc --listFiles. Also - even when in the exclude the package source files pull them in.
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.
yes I probably just messed up.
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.
Well, it all seems to work 🤷♀️ Basically just checking whether we should start to explicitly exclude these files now
e983925    to
    a7a314a      
    Compare
  
    
Description
I found many of the packages were repeating a lot for values that could be lifted to the common tsconfig using a new feature of the TypeScript compiler.
Merging this PR will:
${configDir}template variable introduced in TypeScript 5.5. Since these are settings which are normally overridden by the extending configuration, I don't expect these additions to cause any downstream breakage."allowJs": truefrom packages with no JS to discourage use of JS where TS could be used."strict": trueand"esModuleInterop": truein package tsconfigs, which are inherited from the common config."jsx": "react"settings to simply extend our react-specific config.download-urlpackage with the others, to output into./distinstead of./lib.Open Questions
Checklist