feat(context): improve context listener and events #4451
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 tasks
raymondfeng
force-pushed
the
context-listener
branch
2 times, most recently
from
a74170f to
ee87f36
Compare
jannyHou
reviewed
Jan 17, 2020
| } | ||
| this._parentEventListeners = undefined; | ||
| if (this._parent && this.parentEventListener) { | ||
| this._parent.removeListener('bind', this.parentEventListener); |
There was a problem hiding this comment.
The old code removes all the listeners, any reason why only moves the bind and unbind now?
There was a problem hiding this comment.
The old code uses a map for registered listeners. With this PR, only one function is used on bind/unbind events.
achrinza
added
feature
IoC/Context
bajtos
reviewed
Jan 20, 2020
raymondfeng
force-pushed
the
context-listener
branch
from
ee87f36 to
d3e927f
Compare
|
@bajtos Your comments have been addressed. PTAL. |
raymondfeng
force-pushed
the
context-listener
branch
from
d3e927f to
e3de3bf
Compare
bajtos
approved these changes
Jan 21, 2020
| } | ||
| this.emit('bind', binding, this); | ||
| this.emitEvent('bind', {binding, context: this, type: 'bind'}); |
There was a problem hiding this comment.
Can emitEvent read the event name from event.type?
Suggested change
| this.emitEvent('bind', {binding, context: this, type: 'bind'}); | |
| this.emitEvent({binding, context: this, type: 'bind'}); |
…ling
BREAKING CHANGE: Context events are now emitted as `ContextEvent` objects
instead of positional arguments. Context listener functions must switch from
the old style to new style as follows:
1. Old style
```ts
ctx.on('bind', (binding, context) => {
// ...
});
```
2. New style
```ts
ctx.on('bind', (event: ContextEvent) => {
// ...
});
```
Or:
```ts
ctx.on('bind', ({binding, context, type}) => {
// ...
});
```
raymondfeng
force-pushed
the
context-listener
branch
from
e3de3bf to
c5cd690
Compare
emonddr
approved these changes
Jan 21, 2020
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Spin-off from #4377
ContextEventListenerinterfacemaxListenerstoInfinityby defaultChecklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm testpasses on your machinepackages/cliwere updatedexamples/*were updated👉 Check out how to submit a PR 👈