Skip to content

Conversation

@mschnee
Copy link
Contributor

@mschnee mschnee commented May 21, 2020

There is a small misbehavior in boot.mixin.ts, where upon invoking this.component(ComponentClass), mountComponentBooters will assume "components.${class.name}" to be the binding key, even if another key was specified.

This presents itself in Applications where components marked with @bind() using a custom name, and attempting to use them:

export const COMPONENT_KEY = BindingKey.create<MyComponent>('org.my.namespace.MyComponent');
@bind({
  tags: {[ContextTags.KEY]: COMPONENT_KEY},
  scope: BindingScope.SINGLETON,
})
export class MyComponent implements Component, LifeCycleObserver {
}
import { MyComponent } from '@myScope/my-library';
export class MyApplication extends BootMixin(ServiceMixin(RestApplication)) {
  constructor() {
    this.component(MyComponent);
    // Error: The key 'components.MyComponent' is not bound to any value in context
  }
}

This PR addresses this, by implementing a similar fix to what was done in repository.mixin.ts.

See also #5477

Checklist

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

👉 Check out how to submit a PR 👈

@mschnee mschnee requested a review from jannyHou as a code owner May 21, 2020 22:54
@mschnee mschnee changed the title Fix: boot mixing should use super.component() to get binding key Fix: bootmixin should use super.component() to get binding key May 21, 2020
@mschnee mschnee force-pushed the fix/boot-mixin-with-custom-binding-key branch from f24c414 to 9c039e7 Compare May 21, 2020 22:56
@raymondfeng
Copy link
Contributor

@mschnee Thanks. Please run npm run prettier:fix to fix the formatting issue - https://travis-ci.com/github/strongloop/loopback-next/jobs/338658227

@mschnee mschnee force-pushed the fix/boot-mixin-with-custom-binding-key branch from 4fd4159 to 8e4d0a9 Compare May 22, 2020 16:49
@mschnee
Copy link
Contributor Author

mschnee commented May 22, 2020

@raymondfeng lint complete. Looks good, minus the MacOS test failing.

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

Thank you @mschnee good catch, I left a few suggestions. LGTM 👍
I triggered a new build for the macos test.

@mschnee mschnee force-pushed the fix/boot-mixin-with-custom-binding-key branch 2 times, most recently from b9f1440 to 3e49de2 Compare May 23, 2020 18:19
@mschnee mschnee requested review from jannyHou and raymondfeng May 23, 2020 18:20
@mschnee
Copy link
Contributor Author

mschnee commented May 23, 2020

@jannyHou @raymondfeng I've updated the tests and added documentation, as suggested :)

@mschnee
Copy link
Contributor Author

mschnee commented May 25, 2020

I see that I added some code that has no coverage- I will update this when I'm back at my computer after the holiday weekend :)

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request, @mschnee! ❤️

I quickly skimmed through the changes and they look good at high level. Please try to get approval from other people who commented in this pull request before you proceed to landing.

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@raymondfeng
Copy link
Contributor

@mschnee Ping. Can we fix up the minor issues commented by @bajtos and land it?

@mschnee
Copy link
Contributor Author

mschnee commented May 27, 2020

@raymondfeng good morning! I'm working on this now :)

@mschnee mschnee force-pushed the fix/boot-mixin-with-custom-binding-key branch from 3e49de2 to e5a915c Compare May 27, 2020 17:06
@mschnee mschnee force-pushed the fix/boot-mixin-with-custom-binding-key branch from e5a915c to 6c57478 Compare May 27, 2020 18:14
@mschnee mschnee merged commit 9a855ff into loopbackio:master May 27, 2020
@mschnee mschnee deleted the fix/boot-mixin-with-custom-binding-key branch May 27, 2020 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants