You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by vladimir-kotikov <gi...@git.apache.org> on 2016/02/01 14:33:20 UTC
[GitHub] cordova-lib pull request: CB-10430 Allow to modify events source
GitHub user vladimir-kotikov opened a pull request:
https://github.com/apache/cordova-lib/pull/370
CB-10430 Allow to modify events source
This fixes [CB-10430](https://issues.apache.org/jira/browse/CB-10430).
This change is needed to allow other tools to replace default `EventEmitter` instance to custom one (e.g. platform might want to replace default `EventEmitter` with the one provided by caller in order to pass these events to upstream code)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/MSOpenTech/cordova-lib CB-10430
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-lib/pull/370.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #370
----
commit 66ee6a90ad30b708c1ebcf2abf01773d8ab42c6e
Author: Vladimir Kotikov <v-...@microsoft.com>
Date: 2016-01-12T07:56:59Z
CB-10430 Allow to modify events source
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10430 Allow to modify events source
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-lib/pull/370#issuecomment-181906497
Closing this for now. I'll reopen another PR with updated implementation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10430 Allow to modify events source
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-lib/pull/370#issuecomment-179297923
No, this is not a part of API. However, this might be critical for some external tools to get this information.
@nikhilkh, what do you think about this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10430 Allow to modify events source
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on the pull request:
https://github.com/apache/cordova-lib/pull/370#issuecomment-180043070
In this case, why not emit the event that you are calling spawn with arguments a,b,c
```
function execSpawn(command, args, resultMsg, errorMsg) {
events.emit( ... )
return superspawn.spawn(command, args).then(function(result) {
...
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10430 Allow to modify events source
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-lib/pull/370#issuecomment-180028239
I like the idea of a global.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10430 Allow to modify events source
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-lib/pull/370#issuecomment-178554295
@dblotsky, @purplecabbage, thanks for feedback. Agree, this probably smells fishy. However, the only one option i see apart from this one is to setup a proxy to transmit events from `cordova-common` to another `EventEmitter` instance (which was passed to `PlatformApi` constructor), when necessary.
All other solutions, that came into my mind, break `events` API and hence require some global changes in cordova (as the `events` is used really widely).
However, this PR doesn't change a public API for this and seems to fix the issue with missing events from `cordova-common` internals without affecting any other cases, so it might make sense to get this in as an intermediate fix and then start looking into redesigning cordova `events` mechanics.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10430 Allow to modify events source
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-lib/pull/370#issuecomment-178561020
Because we've been logging these events all the time before cordova refactoring. Many of these events doesn't have much value though, and could be safely ignored, but, for example, the command and arguments, used by superspawn are pretty important information IMO, and there should be a way to get it without using debugger
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10430 Allow to modify events source
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov closed the pull request at:
https://github.com/apache/cordova-lib/pull/370
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10430 Allow to modify events source
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on the pull request:
https://github.com/apache/cordova-lib/pull/370#issuecomment-180030165
I am still not seeing the value in this.
When is this an issue? Who does it affect?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10430 Allow to modify events source
Posted by dblotsky <gi...@git.apache.org>.
Github user dblotsky commented on the pull request:
https://github.com/apache/cordova-lib/pull/370#issuecomment-178861517
Is this a part of our external API? If not, then why should there be a way to get them without the debugger if it's an internal feature?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10430 Allow to modify events source
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on the pull request:
https://github.com/apache/cordova-lib/pull/370#issuecomment-178414449
Yeah, I too am not entirely behind this.
It seems like an anti-pattern here, and a consequence of require('../events') only being executed once per module. This essentially makes it a global object that may change at runtime.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10430 Allow to modify events source
Posted by dblotsky <gi...@git.apache.org>.
Github user dblotsky commented on the pull request:
https://github.com/apache/cordova-lib/pull/370#issuecomment-178305625
Overriding an implementation's logging with your own sounds like a roundabout way to get what you need. This sounds like it's exposing internal events for the purposes of external debugging, which is best solved by using a debugger, not with a custom logging mechanism.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10430 Allow to modify events source
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-lib/pull/370#issuecomment-180026635
Guys, just a couple of ideas, based on discussion with Nikhil:
1, Instead of overwriting `events` instance in common, platform might set a reference to `events` on `global` object, which `cordova-common` will check at runtime and use if available
2. Platform might create a proxy - just subscribe to and redirect `cordova-common`'s `events` to own instance - this one have an advantage - we would not need for any special logic in `common` for that
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10430 Allow to modify events source
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-lib/pull/370#issuecomment-179384632
I understand that this approach is not an ideal pattern - Anyone has better ideas?
We do need this to work to ensure that --verbose logs that we produce have details of activity in cordova-common. In particular, logging of all the commands that we spawn (with arguments). It really helps in diagnosing failures and bugs.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10430 Allow to modify events source
Posted by dblotsky <gi...@git.apache.org>.
Github user dblotsky commented on the pull request:
https://github.com/apache/cordova-lib/pull/370#issuecomment-178555686
@vladimir-kotikov I guess my point is: why is it necessary for internal `cordova-common` events to reach platform code?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10430 Allow to modify events source
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-lib/pull/370#issuecomment-180023878
Guys, just a couple of ideas, based on discussion with Nikhil:
1. Instead of rewriting cordo
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10430 Allow to modify events source
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-lib/pull/370#issuecomment-180036146
https://issues.apache.org/jira/browse/CB-10430 has details of one scenario where verbose logs from the cordova-common do not show up. In particular, the processes that we spawn using superspawn as part of a build can fail and we get an error without context of the command that was run. Ideally, --verbose build logs will have details of the external command that was executed with the correct arguments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10430 Allow to modify events source
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-lib/pull/370#issuecomment-180179865
Yes, we could fix all callers of it - but ideally, 'log' messages from `cordova-common` can be surfaced and reported.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org