fix: adds check forReflect.getOwnMetadataKeys

[ChristianKienle]

To be honest: I am not 100% certain that this change is good because it may simply hide an incorrect configuration on the part of the developer. However since there is already a check in place for the existence of Reflect adding a check for Reflect.getOwnMetadataKeys may also be warranted.

[ktsn]

Why is this change is needed even though we already check Reflect.defineMetadata?

[ChristianKienle]

There are polyfils which implement Reflect.defineMetadata but not Reflect.getOwnMetadataKeys. If this is the case and the consuming app does not manually import reflect-metadata an error will be raised.

I encountered this in the following context:

• Assume there is an existing application using such incomplete polyfill.
• Develop a Vue-plugin that depends on reflect-metadata (because it is using the corresponding vue-cli settings aka class based components).
• Import the plugin in your existing application.

Then the error will be thrown.

This can be solved by the developer importing reflect-metadata manually. But this is not obvious.

I wrote in my initial comment that my change only works around the issue and does not address the root problem (an incomplete Reflect-implementation).

But then I saw that you are already checking for the existence of Reflect and defineMetadata. If those things are not present your fallback is to (gracefully) do nothing. So I thought:

Either we throw an error if anything we need is not present or we also gracefully do nothing.

[ktsn]

There are polyfils which implement Reflect.defineMetadata but not Reflect.getOwnMetadataKeys

Which polyfill is it? I'm suspect we should support such polyfills though as Metadata Proposal includes both API. https://rbuckton.github.io/reflect-metadata/
Supporting such case would bloat the code base as we always need to add such check when we start to use a new api.

Either we throw an error if anything we need is not present or we also gracefully do nothing

We should do nothing as metadata support is optional.

[ChristianKienle]

It is the polyill which is part of Aurelia.

Do you want me to find the actual code?

[ChristianKienle]

I found the code in question:

https://github.com/aurelia/polyfills/blob/master/src/reflect.js

If you search for getOwnMetadataKeys you won't get a hit.

I think Aurelia wants only a partial implementation of the required Polyfills because that is how they define their minimal requirements.

We should do nothing as metadata support is optional.

Yeah. Then raising an error seems to be not an option. I was not 100% sure if metadata is required or not within the context of this project.

Supporting such case would bloat the code base as we always need to add such check when we start to use a new api.

Isn't that just a consequence of using Polyfills in general? Polyfills already bloat the code because they introduce code that will hopefully become unnecessary in the future. While everything is still in flux, being extremely defensive when it comes to optional features introduced by polyfills may be warranted. But I am not sure and obviously you decide. :)

The thing is that I ran into this issue while introducing Vue (class components) in an existing Aurelia project and it took me some time to realize what is going on.

[ktsn]

Polyfills already bloat the code

I'm not talking about bundled code size. I'm talking about more like maintainability.

But I see your usecase and Aurelia's polyfill is reasonable as it is intended only to have minimal code they need. So this kind of change would make sense to me.

Can we move getOwnMetadataKeys check into reflectionIsSupported condition before merge it? It would be simpler as we can keep only watch one constraint whether we should forward metadata or not in that case.
It would be also good to add some comment on reflectionIsSupported to share this context.

[ChristianKienle]

@ktsn Done.

[ktsn]

LGTM. Thank you!