Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(bidi): ensure consistent interface between service and directive #17659

Open
wants to merge 1 commit into
base: master
from

Conversation

@crisbeto
Copy link
Member

crisbeto commented Nov 9, 2019

Currently the bidi module has a Directionality service and a Dir directive. The directive re-provides itself as the service, however it doesn't have the same interface and behavior can cause inconsistencies depending on where it's injected. E.g. the directive has a dir property whereas the service doesn't. Also the directive will emit when it's value is re-assigned, but the service won't.

These changes bring the two closer together by moving the functionality into the service and having the directive extend it.

Somewhat related to #17637, because it means that consumers can't notify the service when the value has changed.

Currently the `bidi` module has a `Directionality` service and a `Dir` directive. The directive re-provides itself as the service, however it doesn't have the same interface and behavior can cause inconsistencies depending on where it's injected. E.g. the directive has a `dir` property whereas the service doesn't. Also the directive will emit when it's value is re-assigned, but the service won't.

These changes bring the two closer together by moving the functionality into the service and having the directive extend it.

/** Stream that emits whenever the 'ltr' / 'rtl' state changes. */
readonly change = new EventEmitter<Direction>();

/** @docs-private */
get dir(): Direction { return this._dir; }
set dir(value: Direction) {

This comment has been minimized.

Copy link
@jelbourn

jelbourn Nov 9, 2019

Member

I don't want to expose a public setter for this since it doesn't actually set the global dir attribute. Additionally, this will emit when the property changes, but bot when the dir attribute actually changes; we would either need to set up a mutation observer for that (which I would hesitate to do for perf reasons) or say that you can only update the global dir via the service

This comment has been minimized.

Copy link
@crisbeto

crisbeto Nov 9, 2019

Author Member

That's why I've made docs-private for now. I'm fine with documenting that the changes will only be picked up if they're done programmatically through the service/directive. Right now they'll be picked up from the directive, but not the service.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.