feat(core-types): Create @rnx-kit/core-types package and consume types from that package#3961
feat(core-types): Create @rnx-kit/core-types package and consume types from that package#3961
Conversation
…s to a new @rnx-kit/core-types package
tido64
left a comment
There was a problem hiding this comment.
In my experience, these kinds of catch-all type packages tend to accumulate unrelated or legacy definitions over time. Without clear, enforced criteria for what qualifies as "core", the package can gradually become a dumping ground for anything that’s shared somewhere, including types tied to features that later change or disappear. When there’s also no explicit policy for ownership, review standards, and deprecation/removal, it becomes hard to keep the package coherent and healthy.
The long-term risk is that the module turns into a tightly coupled dependency hub: changes become high-blast-radius, the package becomes intimidating to modify, and it eventually slows down refactoring because it's easier to add "just one more type" than to untangle things properly.
Instead, I think we should prefer package-/domain-specific type packages. If another package needs to consume types from @rnx-kit/config, we should publish something like @rnx-kit/config-types and keep the scope intentionally narrow and owned by the same domain. This keeps dependencies and responsibility clear, avoids a single "everything depends on this" package, and allows types to evolve with the package they represent.
If we go this route, the guideline could be:
- No centralized types package.
- Types should live with their owning package, and only be extracted into a *-types package when there’s a clear consumer need outside that package.
- Each *-types package should have an explicit scope tied to that domain (e.g., config-related types only).
… package manifests into dedicated packages"
Makes sense. I've split the types packages into three more specific packages in the latest iteration. |
…as they fail without builds
| /** @deprecated Use `types` instead */ | ||
| typings?: string; |
There was a problem hiding this comment.
We should just remove this.
| /** @deprecated Use `types` instead */ | |
| typings?: string; |
There was a problem hiding this comment.
I want to leave it in as it is something that is worth using constraints or package linting to migrate/flag if you have it set.
There was a problem hiding this comment.
We can still check whether typings is present in the manifest regardless. We should remove it so that people can't use it.
| "@rnx-kit/types-kit-config": "^0.0.1", | ||
| "@rnx-kit/types-node": "^0.0.1" |
There was a problem hiding this comment.
For dev dependencies, we should use * for now until we figure out a way to use workspace:*
| "@rnx-kit/types-kit-config": "^0.0.1", | |
| "@rnx-kit/types-node": "^0.0.1" | |
| "@rnx-kit/types-kit-config": "*", | |
| "@rnx-kit/types-node": "*" |
There was a problem hiding this comment.
I still don't think it is correct to lump all plugins together like this. It is now too easy for someone to "see patterns" and make them interconnected. We should either keep them within the plugin package, or create separate types packages. I don't believe the number of packages in the repo is going to dominate our CI times.
| { | ||
| "extends": "@rnx-kit/tsconfig/tsconfig.node.json", | ||
| "compilerOptions": { | ||
| "emitDeclarationOnly": true |
There was a problem hiding this comment.
I still don't understand this. If this is a types only package, why do we need to emit anything at all? Can we not just consume the TypeScript?
| /** @deprecated Use `types` instead */ | ||
| typings?: string; |
There was a problem hiding this comment.
We can still check whether typings is present in the manifest regardless. We should remove it so that people can't use it.
There was a problem hiding this comment.
This should be a separate PR. Let's not scope creep.
Description
This change creates a new
@rnx-kit/core-typespackage for core configuration types, manifest types, lint types, and so on. This avoids pulling in implementations when not needed and generally cleans up the dependency graph.There are also several places where using rnx-kit packages via an API from outside rnx-kit weren't exposing types correctly because the types came from a devDependency package (because the implementation wasn't needed) but as a result the types weren't being exported correctly. Having a lightweight types package allows for these to be correctly marked as dependencies causing the types to resolve correctly from outside the repo.
KitConfig and Supporting Types
The
KitConfigwas coming out of the@rnx-kit/configpackage which was pulling in several metro plugin packages for the types. Some plugin types were defined in the config package, some were defined locally, regardless it created a bit of a dependency snarl. These were the primary types moved into the new package.Package Manifest Types
The
PackageManifesttype has been moved from tools-node into core-types. It has also been greatly improved by:I also added a
PackageDatatype which is designed as a parameter type for functions that want to take a folder and manifest pairing which can be fulfilled byPackageInfoor by other implementations. This will be used in subsequent changes.AllPlatforms
I also moved the
AllPlatformstype here, and changed it to be based on an array definition ensuring those stay in sync.Test plan
This is almost 100% type only (with the exception of the
ALL_PLATFORM_VALUESarray so automated tests + pipelines should be sufficient.