inversify / InversifyJS

A powerful and lightweight inversion of control container for JavaScript & Node.js apps powered by TypeScript.
http://inversify.io/
MIT License
11.34k stars 719 forks source link

Consider turning the TAGGED metadata value into a Map #1637

Closed mikeki closed 2 weeks ago

mikeki commented 2 weeks ago

Is there an existing proposal similar to this?

What are you proposing?

Currently the TAGGED metadata is an Array of Metadata objects. This means that we need to iterate over that array and compare the metadata.key attribute against the key of the metadata we want to pull.

Consider turning this Array into a Map for constant time lookup.

Is there any specific group of users that will benefit from this?

everyone implementing custom metadatas.

What problems are you trying to solve?

Adding custom metadatas and fetching them means iterating over the full array, that's an efficiency problem.

Do you have any references or examples that can illustrate your idea?

The problem:

https://github.com/tsinatra/tsinatra/blob/main/inject/src/annotation/BindingDecorator.ts#L51-L62 https://github.com/tsinatra/tsinatra/blob/main/serializer/src/JsonPropertySerializer.ts#L122-L130

What type of idea is this?

Innovation: No similar idea exists

notaphplover commented 2 weeks ago

Hey @mikeki, we are on our way to improve our metadata model :smiley:

The biggest refactor going on under the scenes is preciselly that: rely on better metadata data structures. The internal models inversify is using internally to represent a class element metadata is ClassElementMetadata.

We need to be extra careful there: METADATA_KEY, Metadata, Target and Request are exposed. We need to find a way to break in a reasonable way so upgrading to the next major is not crazy for users relying on our metadata or middlewares, like your project.

So, answering to this feature request, we won't be providing a metadata map. Instead, we will rely on ClassMetadata and ClassElementMetadata (which rely on Map) in the future. I will need some time to do it in order to give everyone proper support answering requests and fixing bugs, but maybe in a month or so we will be close to it.

mikeki commented 2 weeks ago

That's great to know!

My project is not currently being used in production anywhere so it's okay to break it.

One other question, I know that currently inversify relies on reflect-metada, but TC39 is coming close to the new standard for decorators.

What's your plan for that?

On Thu, Nov 14, 2024, 2:22 PM notaphplover @.***> wrote:

Hey @mikeki https://github.com/mikeki, we are on our way to improve our metadata model 😃

The biggest refactor going on under the scenes is preciselly that: rely on better metadata data structures. The internal models inversify is using internally to represent a class element metadata is ClassElementMetadata https://github.com/inversify/monorepo/blob/main/packages/container/libraries/core/src/metadata/models/ClassElementMetadata.ts .

We need to be extra careful there: METADATA_KEY, Metadata, Target and Request are exposed. We need to find a way to break in a reasonable way so upgrading to the next major is not crazy for users relying on our metadata or middlewares, like your project.

So, answering to this feature request, we won't be providing a metadata map. Instead, we will rely on ClassMetadata and ClassElementMetadata (which rely on Map) in the future. I will need some time to do it in order to give everyone proper support answering requests and fixing bugs, but maybe in a month or so we will be close to it.

— Reply to this email directly, view it on GitHub https://github.com/inversify/InversifyJS/issues/1637#issuecomment-2477338037, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACH7F3QR3Q73HRTNMMA5SD2AUA75AVCNFSM6AAAAABRZSKG4WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINZXGMZTQMBTG4 . You are receiving this because you were mentioned.Message ID: @.***>

notaphplover commented 2 weeks ago

Until there's support for parameter decorators we will keep using legacy ones. There's a discussion going on :smiley: .