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(strongly-typed): initial commit #59

Merged
merged 2 commits into from
Nov 16, 2024

Conversation

alecgibson
Copy link
Contributor

Add an initial offering for @inversifyjs/strongly-typed, which will let consumers optionally apply strong typing to their Container.

Copy link
Member

@notaphplover notaphplover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks great.

I added some suggestions, what do you think about them?

Copy link
Member

@notaphplover notaphplover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks great!

I think we're almost done, we just need to handle this scenario and we're ready to merge it:

it('gets a promise', async () => {
  // @ts-expect-error :: can't call get() to get Promise
  expect(() => container.get('asyncNumber')).toThrow(
    'it has asynchronous dependencies',
  );
  const n: Promise<number> = container.getAsync('asyncNumber'); // tsc complains
  expect(await n).toBe(1);
});

@ffMathy
Copy link

ffMathy commented Nov 14, 2024

This is amazing.

Not sure why this is not offered by default. A lot of people are not going to find this when it's an extension, and TypeScript is almost used everywhere at this point.

Can you elaborate on the thoughts of not having this in the main project? The generic type could just default to "any" or something similar.

@notaphplover
Copy link
Member

notaphplover commented Nov 14, 2024

Hey @ffMathy, it's a long story. I could resume it in these bullet points:

  • I really want to avoid adding complexity in the main repository as much as possible until the monorepo migration is ready.
  • This approach is amazing, but the logical enhancements around it suggests a config based bindings which are not exactly aligned with the code driven bindings original design. Keeping this out of the main repo allows us to carefully see how it evolves without introducing a feature in the main repo.
  • The approach is practical for most cases, but it feels incomplete when some advanced tools like middlewares are involved. Keeping it out of the repo allows this type system to not to handle every impossible edge case you might face when using inversify. As a plugin, the plugin docs might perfectly have a scope section in which known limitations are ellaborated.

As said, this is just a summary.

Regarding visibility, I will update docs to give the visibility it deserves 😃

Copy link
Member

@notaphplover notaphplover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I saw a last minute issue, adding the comment.

I was about to do this myself but, would you mind adding an ESM build the same way we're actually doing with other modules? Otherwise I'll do it after we merge this.

Thank you so much for your great job!

Comment on lines 21 to 22
: TKey extends interfaces.Abstract<infer D>
? D
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see this in preview reviews..., sry. Ok, interfaces.ServiceIdentifier<T> no longer includes interfaces.Abstract<T>. Instead, it includes the expected type Function. Since Abstract<T> was just a trick to exploit the any type of a Function, what about this:

Suggested change
: TKey extends interfaces.Abstract<infer D>
? D
: TKey extends Function
? any

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay not entirely sure I follow, but have done this anyway since it matches upstream.

@alecgibson
Copy link
Contributor Author

@notaphplover without changing anything my build now fails locally:

❯ pnpm test

> @inversifyjs/[email protected] test /Users/alec/git/work/reedsy/inversify/monorepo/packages/container/libraries/strongly-typed
> jest --config=jest.config.mjs --runInBand

● Validation Error:

  Module ts-jest in the transform option was not found.
         <rootDir> is: /Users/alec/git/work/reedsy/inversify/monorepo/packages/container/libraries/strongly-typed

  Configuration Documentation:
  https://jestjs.io/docs/configuration

 ELIFECYCLE  Test failed. See above for more details.

Did we change something upstream that I need to adapt to?

The overly-aggressive commit hooks won't let me commit without fixing this.

@notaphplover
Copy link
Member

Did we change something upstream that I need to adapt to?

Hey @alecgibson , everything seems to be fine in th CI environment and mine.

The overly-aggressive commit hooks won't let me commit without fixing this.

git hooks are client side so, at the end, are optional. You can bypass then using the --no-verify option:

git commit --no-verify

@alecgibson alecgibson force-pushed the strong-types branch 2 times, most recently from 31fae7d to 0727255 Compare November 15, 2024 09:55
Add an initial offering for , which will let consumers optionally apply strong typing to their .
@alecgibson
Copy link
Contributor Author

alecgibson commented Nov 15, 2024

You can bypass then using the --no-verify option

Thanks! Didn't know about this. (I've always found commit hooks a bit heavy-handed for my personal taste 🤷🏼 ).

I've made the requested changes (I think? I wasn't sure what you meant about the ESM build; I just copied the webpack config and script from core).

Want to also make sure you're happy with the location of this library in the folder structure?

@ffMathy
Copy link

ffMathy commented Nov 15, 2024

So exciting! Great work everyone! 😍

@notaphplover notaphplover merged commit f95e6ca into inversify:main Nov 16, 2024
3 checks passed
@alecgibson alecgibson deleted the strong-types branch November 16, 2024 07:01
@ffMathy
Copy link

ffMathy commented Nov 16, 2024

The package seems to not be public somehow?

If I go to https://www.npmjs.com/package/@inversifyjs/strongly-typed, I am taken to a login page.

@notaphplover
Copy link
Member

Hey @ffMathy, the package is not ready to be published. Additional PR are required to provide with inject decorators relying on a BindingMap

@ffMathy
Copy link

ffMathy commented Nov 16, 2024

Okay. Is this being worked on and tracked anywhere? And if not, is that something I can work on?

@alecgibson
Copy link
Contributor Author

For what it's worth I personally consider this TypedContainer feature "useful enough" as its own thing to release it without the strongly-typed @inject() decorators, which would be a minor-versioned, non-breaking update anyway. I'll work on that hopefully this week when I get some time.

alecgibson added a commit to alecgibson/monorepo that referenced this pull request Nov 19, 2024
This change builds on the work of the [`TypedContainer`][1] by adding
type definitions for a strongly-typed `inject` decorator.

[1]: inversify#59
alecgibson added a commit to alecgibson/monorepo that referenced this pull request Nov 19, 2024
This change builds on the work of the [`TypedContainer`][1] by adding
type definitions for a strongly-typed `inject` decorator.

[1]: inversify#59
@alecgibson
Copy link
Contributor Author

inject typings now raised here: #114

alecgibson added a commit to alecgibson/monorepo that referenced this pull request Nov 20, 2024
This change builds on the work of the [`TypedContainer`][1] by adding
type definitions for a strongly-typed `inject` decorator.

[1]: inversify#59
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