Conversation
YvetteNikolov
requested review from
FreakyWizard,
JasperS20,
WybeBosch,
Yannicvanveen,
Copilot,
hnccox-yard,
laravdiemen and
rivanuff
There was a problem hiding this comment.
Pull request overview
Adds a new @yardinternet/ts-config workspace package to centralize shared TypeScript tsconfig presets for Yard projects (e.g., Brave sites and publishable libraries).
Changes:
- Introduces base, Brave, and library TypeScript config JSON files under
packages/ts-config/. - Adds package metadata + usage docs for the new
@yardinternet/ts-configpackage. - Registers the new workspace in the monorepo and updates the root lockfile accordingly.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ts-config/tsconfig.library.json | New tsconfig preset intended for library/npm package builds. |
| packages/ts-config/tsconfig.brave.json | New tsconfig preset intended for Brave TypeScript projects. |
| packages/ts-config/tsconfig.base.json | New shared base tsconfig with strictness/module settings and default excludes. |
| packages/ts-config/package.json | Adds a publishable workspace package definition for @yardinternet/ts-config. |
| packages/ts-config/README.md | Documents how consumers should extend the provided tsconfigs. |
| package.json | Adds packages/ts-config to workspaces. |
| package-lock.json | Updates lockfile to include the new workspace and its dependency tree. |
| "repository": { | ||
| "type": "git", | ||
| "url": "git+https://github.com/yardinternet/toolkit.git", | ||
| "directory": "packages/vite-config" | ||
| }, |
There was a problem hiding this comment.
The repository.directory field is set to packages/vite-config, which is incorrect for this package and will mislead links shown on npm/GitHub Packages. Update it to packages/ts-config.
| "extends": "./tsconfig.base.json", | ||
| "compilerOptions": { | ||
| }, | ||
| "exclude": ["node_modules", "vendor", "public", "plugins"] |
There was a problem hiding this comment.
This exclude array replaces (does not merge with) the base config’s exclude because of extends, which means dist/build etc from tsconfig.base.json will no longer be excluded for Brave projects. Consider removing exclude here to inherit the base list, or include the base exclusions plus any Brave-specific ones.
| "compilerOptions": { | ||
| // Base | ||
| "esModuleInterop": true, // Allow default imports from CommonJS | ||
| "jsx": "react", // Classic in stead of react-jsx, allows JSX without react installed as dependency. |
There was a problem hiding this comment.
Typo in comment: “in stead” should be “instead”.
| "compilerOptions": { | ||
| // Base | ||
| "esModuleInterop": true, // Allow default imports from CommonJS | ||
| "jsx": "react", // Classic in stead of react-jsx, allows JSX without react installed as dependency. |
There was a problem hiding this comment.
The comment on the jsx setting appears inaccurate: "jsx": "react" uses the classic React transform and typically requires a React runtime (and often an import in scope). If the goal is JSX without requiring React at runtime, this should use the automatic runtime (react-jsx) or a non-React JSX emit with the appropriate jsxImportSource/factory settings.
| { | ||
| "name": "@yardinternet/ts-config", | ||
| "version": "1.0.0", | ||
| "main": "index.js", |
There was a problem hiding this comment.
main points to index.js, but this package currently doesn't include an index.js file. Either add the referenced entrypoint or remove/adjust main so consumers don't get a broken import/require resolution.
No description provided.