Skip to content

Conversation

@bajtos
Copy link
Member

@bajtos bajtos commented Sep 2, 2019

See the discussion in #3617

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

@bajtos bajtos added the IoC/Context @loopback/context: Dependency Injection, Inversion of Control label Sep 2, 2019
@bajtos bajtos requested a review from raymondfeng September 2, 2019 09:40
@bajtos bajtos self-assigned this Sep 2, 2019
@bajtos bajtos force-pushed the fix/context-markParameterOrPropertyAsInjected branch from a19af75 to d7c4001 Compare September 2, 2019 09:41
return function markParameterOrPropertyAsInjected(
target: Object,
member: string,
member: string | undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

The ParameterDecorator type from TypeScript is declared as follows:

declare type ParameterDecorator = (target: Object, propertyKey: string | symbol, parameterIndex: number) => void;

Can we use '' for constructor parameters?

Copy link
Contributor

Choose a reason for hiding this comment

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

Interestingly, at runtime, the propertyKey is undefined in constructor cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interestingly, at runtime, the propertyKey is undefined in constructor cases.

That's the weird thing!

Copy link
Contributor

@raymondfeng raymondfeng left a comment

Choose a reason for hiding this comment

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

I'm fine to merge this PR as the undefined is given for member at runtime for the constructor even though it's different from the type definition. Maybe we should report the consistency to TypeScript.

…args

Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>
@raymondfeng raymondfeng force-pushed the fix/context-markParameterOrPropertyAsInjected branch from d7c4001 to e1ecd2c Compare September 4, 2019 16:28
@bajtos bajtos merged commit 6a0d4f2 into master Sep 5, 2019
@bajtos bajtos deleted the fix/context-markParameterOrPropertyAsInjected branch September 5, 2019 11:56
@bajtos
Copy link
Member Author

bajtos commented Sep 5, 2019

Maybe we should report the consistency to TypeScript.

I was considering that, but was lazy to fill the report 🙈

Here we go: microsoft/TypeScript#33260

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

IoC/Context @loopback/context: Dependency Injection, Inversion of Control

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants