Support type inference - #408
Conversation
|
@jeffijoe Maybe you would consider removing the lockfile? It is bringing pain and misery on every PR :-/ |
|
Its weird because all I did was npm install locally, why does it keep breaking? |
|
@jeffijoe you need to use older node version, like 20.20.0. newer ones bundle npm that generate format incompatible with older npm. |
|
@jeffijoe Any thoughts on the feature itself? |
|
To be honest, I don't like that we're optimizing for a typed cradle. One of the core principles of Awilix is that it should be transparent to your application code. Even if using Proxy mode, you should be defining a With that said, since we already have partial support for it, then I would rather push a new major version by changing the default cradle type from Also, please tell your Claude to avoid adding unnecessary tests for stuff that is already covered, and the are unnecessary. 😉 I'm perfectly fine with using AI to help write code, but there's a lot of new code added via tests here, and I see a lot that appears redundant (testing runtime functionality that is already covered by existing tests). Then there are tests that (to me) make no sense as to what they are testing? it('preserves generic class type parameters', () => {
class Repository<T> {
items: T[] = []
add(item: T) {
this.items.push(item)
}
}
// asClass can't infer the generic, so use explicit type parameter
const resolvers = {
repo: asClass<Repository<string>>(
Repository as new (...args: any[]) => Repository<string>,
),
}
type Result = InferCradleFromResolvers<typeof resolvers>
assertType<IsExact<Result['repo'], Repository<string>>>(true)
})Like, what exactly are we testing here? It looks like Claude started writing the test, then realized that TS can't actually infer it, then does |
Awesome, I'll rework the PR then!
The intent there is not to cover runtime functionality at all, it is focused on type inference, which I don't think existing tests are covering?.. I will revisit them again to check if all the ones that we have make sense and cover valid inference cases. |
|
@jeffijoe Please check now! |
f502792 to
04471ea
Compare
jeffijoe
left a comment
There was a problem hiding this comment.
Please undo those import changes - I have a feeling you were trying to (or your agent was trying to) fix the type import I suggested. You can use a type specifier in the same import block, you don't need a separate import.
|
There was just 1 comment left (re: |
042f4a8 to
ed3d1e9
Compare
|
@jeffijoe what do you think about bumping minimum Node version for the next semver major? |
|
Too late, already published v13. 😅 |
|
@jeffijoe it would be a documentation and CI change anyway, so version release shouldn't matter :D |
|
Fair enough, go for it. |
This PR offers a way to decrease amount of boilerplate involved in typing dependencies available in the project for the injection.
It would be awesome to provide one more option, that could even replace
createContainerWithentirely. However, it would be a breaking change typing-wise.Here is the proposal:
Typed register() That Returns a Narrowed Container
Make register() return a new container type that includes the registered types:
Usage:
Pros: Works with the existing API, supports incremental registration (chain multiple .register() calls and the type
accumulates). Most ergonomic for the chaining pattern.
Cons: This is a breaking change - current register() returns
this(same container type), changing it to return a widertype changes the contract. Also, the runtime object is the same container (mutated), but the type would appear to be
a new container. Users assigning to a pre-typed variable would get errors. Could be mitigated by making it additive
(
Cradle & InferCradle<R>), but whenCradle = any, the intersection collapses toany.The
anydefault problem is significant:any & { userService: UserService } = any. So this only works if users startwith
createContainer<{}>()instead of the current createContainer() (which defaults Cradle toany).