Symbol resolution fails when importing code across compilation units

[dmtaub]

In trying to use this library on a project that has a build with multiple output javascript files, I find that I am unable to inject or get() dependencies that were defined in a different context, even though during runtime I'm passing the DI Container and valid keys into the other context.

It turns out that the library's internal constants are defined as symbols which are unique per execution context. Short of passing around the library objects (decorators, container, etc) and defining injectables inside closures or assigning them to globals at the beginning of each execution context, there's no way to share the container across compilation units in a way that doesn't fail to inject/get the dependencies.

One simple solution would be to change the constants to be defined as strings, and--in fact--that's what I do in my fork of this library, but I didn't want to open a PR before understanding if there might be another reason for the choice of Symbol for the internal constants?

[pcafstockf]

I do not remember if I chose symbols simply because I wanted to reduce property clutter in the objects they were attached to, or if I was just following a pattern I found WRT the initial implementation of reflect-metadata.

Off the top of my head, your suggestion of switching to strings seems reasonable and good.

I think I would probably rework the names a little (maybe the old school '_' prefix), and would probably want to define them as non-enumerable properties so that folks who might currently be iterating objects, don't suddenly find new properties they were not expecting.
I think these all get bound to the prototype, so that may help avoid iteration surprises.

I'll take a deeper look this weekend.
I only ever use reflect-metadata myself, so I also want to verify such a change would not break the other two Reflect APIs the library supports.

You obviously have a good grasp on the subject to have tracked this down, so if you have any thoughts on the above, please share.

[dmtaub]

I can't take credit for finding the root cause--that was a colleague--though I did create failing examples for him to work with. :)

Once he pointed out what was probably going on, I smacked my head, said "of course," and created this branch to solve my immediate need (the postinstall allows me to use the repo as a package.json dependency directly).

Good thought on checking libraries aside reflect-metadata (I'm using that as well...) - curious whether there's an easy way to add tests for other libs, since you can't easily use multiple shims at the same time ?

As for the names, I did in fact use the more pythonic __name__ format, just to limit the chance someone would prefix an underscore and we'd get a collision, but I hadn't considered making the properties non-enumerable, which wouldn't hurt.

I love how this library focuses on the asynchronous use case, and does so tersely. Thanks for responding so quickly! LMK if I can do something else to help

[pcafstockf]

Its been so long since I dug into the details, that I forgot the purpose of Reflect.setMetadata is to associate meta-data with something, not set data on it.
Indeed, TypeScript itself associates the design:paramtypes and design:returntype meta-data.

So, not only is it safe to make these strings (as TypeScript itself does for design:paramtypes and design:returntype), but I took inspiration from that and also name-spaced them to ensure there will never be collisions.

I believe the theory is solid, and all the tests pass.
I don't see any backwards compatibility concerns, because as you point out, the symbols prevented shared compilation units previously.

Technically I suppose its a 'bug' fix, but going to push it shortly as a 'feature' release just because it does enable support for something that was not previously possible.