(feat) arguments for injection

[vic1707]

Hi,

Just tried to add the feature, seems like it will work well. For now I only did with resolve and not really typesafe (see the unit tests) but unit tests seems to pass without any trouble 😁. Will continue to work on it to adapt the rest of the package and then try to add typeSafety.

[vic1707]

Now the arguments can be passed with wire, the inject decorator. I had to change the array of symbols (NOCACHE & NOPLUGINS) to a real array instead of a deconstructed one (since I used the deconstructed one for the arguments for the injected dependency).

@hbroer, do you think the arguments for the injected dependency should be a real array instead of a deconstructed on ? if so, should I restaure the deconstructed one for the Symbols ? Should I keep both as real arrays and put them in a different order ? 🤔 I think those questions are up to you 😉

Since it seems to be working as it is I'll work on type safety to see if it's possible :grin:.

[hbroer]

Hi, thanks for the work. I will take a look into it withing the next week :-)

[vic1707]

I'm also working on another pr to hopefully add some documentation, reduce build size, and add a little bit more typesafety Once this PR (#58) is done I'd come back here for the typesafety.

[vic1707]

So sad we can't disable tasks on draft PRs

[vic1707]

for now typesafety is ensured by the user because it's the only solution I found for now that doesn't imply huge modifications

class A {
  constructor(public age: number) {
    console.log(age++)
  }
}

const factWith = (age: number) => new A(age);

const TOKEN = {
  factoryWithArgs: token<A>('factoryWithArgs'),
  classWithArgs: token<A>('classWithArgs')
}

const container = new Container();
const resolve = createResolve(container);

container.bind(TOKEN.factoryWithArgs).toFactory(factWith);
setBindedArguments<A, Parameters<typeof factWith>>(TOKEN.factoryWithArgs); // these lines enforces typesafety
container.bind(TOKEN.classWithArgs).to(A);
setBindedArguments<A, ConstructorParameters<typeof A>>(TOKEN.classWithArgs); // these lines enforces typesafety

crap forgot the unit tests, seems like I enforced typesafety which broke the tests because of the MaybeToken type 😓

[vic1707]

tests are fixed but coverage isn't 100% due to the new typeguard, I'll fix this soon 😉

[vic1707]

For now this is the best I can do for typesafety :

• a typeguard that asserts the new type but
  • doesn't work with object
  • doesn't work when the token is made safe and then used inside a class definition => return to Token type for some reason
• typecasting once you created the resolver, the wire or the decorator

here is a snippet, uncomment either one of the two line to add typesafety

import {Container, token, setBindedArguments, createResolve} from './src'

const factWith = (age: number) => age;

const factoryWithArgs = token<number>('factoryWithArgs');

const container = new Container();
const resolve = createResolve(container);

container.bind(factoryWithArgs).toFactory(factWith);

// setBindedArguments<number, Parameters<typeof factWith>>(factoryWithArgs);

const resolvedElement = resolve(factoryWithArgs)
// const resolvedElement = resolve<number, Parameters<typeof factWith>>(factoryWithArgs)

const num = resolvedElement("wrong argument");
console.log(num);

I tried running the typeguard setBindedArguments inside the Bind class since it has all the required infos but unfortunately the type isn't updated in the global scope.

I still need to update the README if everythinfg looks good to you

Build infos

Current `master` build size (M) : ``` Build "@owja/ioc" to dist: 861 B: ioc.js.gz 734 B: ioc.js.br 867 B: ioc.mjs.gz 742 B: ioc.mjs.br ``` ------- New build size (NBS) : ``` Build "@owja/ioc" to dist: 933 B: ioc.js.gz (+72 B ; + 8.36%) 802 B: ioc.js.br (+68 B ; + 9.26%) 943 B: ioc.mjs.gz (+76 B ; + 8.77%) 825 B: ioc.mjs.br (+83 B ; +11.19%) ``` -------- For the day we remove `MaybeToken` (as always for pure informations 😉) ``` Build "@owja/ioc" to dist: 901 B: ioc.js.gz (over NBS: -32 B ; - 3.43%) | (over M: +40 B ; + 4.65%) 773 B: ioc.js.br (over NBS: -29 B ; - 3.62%) | (over M: +39 B ; + 5.31%) 914 B: ioc.mjs.gz (over NBS: -29 B ; - 3.08%) | (over M: +47 B ; + 5.42%) 794 B: ioc.mjs.br (over NBS: -31 B ; - 3.76%) | (over M: +52 B ; + 7.01%) ```

[hbroer]

I think token() should know the arguments because it should be the only source of the type of the service. If thats true than only a factory which has the correct signature can be added as the service.

token<ReturnType, [Arg1Type, Arg2Type, Arg3Type...]>(string);

[vic1707]

I think token() should know the arguments because it should be the only source of the type of the service. If thats true than only a factory which has the correct signature can be added as the service.

token<ReturnType, [Arg1Type, Arg2Type, Arg3Type...]>(string);

Would make many things easier I think. But wouldn't that cause a potential issue if someone wants to rebind a token with a different factory (not that I see a case where anyone would want this 🤔) ? Anyways I'll try this approach.

[vic1707]

new Build size (over master)

Build "@owja/ioc" to dist:
        920 B: ioc.js.gz    (+59 B ; + 6.85%)
        786 B: ioc.js.br    (+52 B ; + 7.08%)
        928 B: ioc.mjs.gz   (+61 B ; + 7.04%)
        804 B: ioc.mjs.br   (+62 B ; + 8.36%)

[vic1707]

@hbroer how about now?

[hbroer]

I am busy atm but I will take a look into it within the next days.

[vic1707]

No problem, sorry if I disturbed you 😰

[vic1707]

Hello @hbroer Do you got any update on this? It's been 2 months since you've said you'll look into it. I know maintaining repo can be pretty demanding. On my part I want to use your library for a project of mine, and it can't be done as cleanly as I want without this PR so please tell me if you don't have the time to check this anytime soon so I can clean my code.

[hbroer]

Hi, sorry but I still had no time to look deeply into it. I merged it and I will publish a new alpha version, so you can clean up a bit.

I may later change a little bit. I want it backwads compatible to 1.0, so the tags need to be arrays or just one symbol. This breaks currently existing apps which use the NOCACHE symbol. It also affects older alphas which use SUBSCRIBE (which affects me ;-) ). But because this is alpha stage it is no problem to change it with a later update.

Thank you for your awesome work!

[vic1707]

Thx for the merge and the new beta version !

I'll take a look at backward compatibility to see if something can be done because with the current code it will be difficult since

which (I think) will prevent the previous syntaxe. If we can implement some sort of method overloading it might be doable 🤔

[vic1707]

this seems to do the trick but only accepts one tag

export function createDecorator(container: Container) {
    return <T, K extends Array<unknown>>(
        token: Token<T, K> | MaybeToken<T>,
        tags: symbol[] | symbol = [],
        ...injectedArgs: K
    ) => {
        return function <Target extends {[key in Prop]: T}, Prop extends keyof Target>(
            target: Target,
            property: Prop,
        ): void {
            define(target, property, container, token, Array.isArray(tags) ? tags : [tags], ...injectedArgs);
        };
    };
}

createDecorator(new Container())(Symbol.for("test"), NOPLUGINS, 1, 2, 3);
createDecorator(new Container())(Symbol.for("test"), [NOPLUGINS], 1, 2, 3);

and I find the Array.isArray really ugly

[hbroer]

it can be done in define itself. Array.isArray(tags) ? tags : [tags] or typeof tags === "symbol" ? [tags] : tags is ok IMO. That's not uncommon ;-) We can't keep backwards compatiblity without it.

[vic1707]

with both versions a new test is required in order to get the 100% coverage, which one do you prefer? The typeof version is lighter once built

-- typeof
Build "@owja/ioc" to dist:
        931 B: ioc.js.gz
        797 B: ioc.js.br
        939 B: ioc.mjs.gz
        818 B: ioc.mjs.br

-- Array.isArray
Build "@owja/ioc" to dist:
        936 B: ioc.js.gz
        803 B: ioc.js.br
        945 B: ioc.mjs.gz
        824 B: ioc.mjs.br

I can do a quick commit if you want to merge it now

edit: the built numbers are from an ioc version where only createDecorator was updated for backward compatibility.