Skip to content
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

feat: validate dependency types in registry #6

Merged
merged 2 commits into from
Sep 22, 2024

Conversation

marcvincenti
Copy link
Contributor

When there are type conflicts on the injectables, we get only an error when providing the api.

In this PR, we propose to typecheck the registry when creating moduleFactory. I don't see any reason (is there any ?) to inject a dependency with a different type than the one expected by other dependencies. That's why we should be able to get an error when instantiating a moduleFactory.

package-lock.json Outdated Show resolved Hide resolved
@lorenzofox3
Copy link
Owner

lorenzofox3 commented Sep 17, 2024

Thanks Marc. I will have a look at it soon.

I don't see any reason (is there any ?) to inject a dependency with a different type than the one expected by other dependencies

A usecase which might be problematic is "optional dependencies".

You can't really declare optional dependencies, so you will need an entry in the registry for that injection token with the undefined value|type. Maybe you can declare you type in that way too but you will probably need to doublecheck your PR does not introduce a regression

const createRepository = ({session}:{session?:Session}) => {} 


const createModule = createProvider({
    injectables: {
        session: undefined // this is required or it will throw at runtime
    }
})

@marc-indy marc-indy force-pushed the feat_registry_validation branch from 7a2a22c to c24bf62 Compare September 17, 2024 14:56
@marc-indy marc-indy force-pushed the feat_registry_validation branch from c24bf62 to a404bdf Compare September 17, 2024 14:59
@lorenzofox3 lorenzofox3 merged commit c5716a4 into lorenzofox3:main Sep 22, 2024
1 check passed
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.

3 participants