support regex for event type #9

bonashen · 2016-12-13T10:15:41Z

support regex for event type
support multi event types for listener

support multi event types for listener

Event object add getArg function.

krasimir · 2016-12-14T21:17:47Z

That's an awesome PR. Let me spent some time next few days to review it.

bonashen · 2016-12-17T03:58:26Z

Hello @krasimir , I tried to increase the event redirection function for EventBus, the purpose is to enable the event to meet the redirection conditions as the target event. Target event types support variability and latency, and can be determined when redirection occurs.

krasimir

@bonashen First of all I want to say that this is massive work. You basically rewrote the library. However, I'm not sure that this is the implementation that I want. I'm happy with the API and and how the EventBus behaves. I'll suggest, let's first start with bunch of test cases that cover all the functionalities.

(To be honest I didn't put lots of efforts to this repo. It was created ages ago and it has 0 test coverage)

krasimir · 2016-12-17T14:58:39Z

src/EventBus.js

@@ -1,100 +1,413 @@
+/**
+ * Created by bona on 2016/12/12.
+ */


Isn't it better to include that in a CHANGE.log file.

I'm sorry. The first time I found the EventBus library, I copied the code directly and pasted it into the project file created by IDEA without noticing the copyright information. Later, it was necessary to give feedback on the changes I made, the code will be submitted directly to the git.

krasimir · 2016-12-17T14:59:14Z

src/EventBus.js

+    function iterator(array, callback) {
+        array = array || [];
+        var iterator = {
+            stop: function (result) {


Can we please add isStopped: false and result: null here as initial values.

krasimir · 2016-12-17T15:02:18Z

src/EventBus.js

+
+    function slice(array, start, end) {
+        if (start > array.length)return [];
+        return [].slice.call(array, start == undefined ? 0 : start, end == undefined ? array.length : end) || [];


Can we use tripple === here.

krasimir · 2016-12-17T15:15:33Z

src/EventBus.js

+         * @returns {EventBusClass}
+         */
+        on: function (type, callback, scope) {
+            type = processMultiTypes.call(this, type, this.on, slice(arguments));


Nice idea about extending the API and make possible handling multiple types. However, processMultiTypes is really really confusing. I mean it took me some time to understand that we call processMultiTypes in every case and in those cases where we have multiple types we return an instance of EventBusClass and because of that this function on stops here (if (type == this)return this; looks weird too). processMultiTypes defines two logical branches that are too different.

I still want that feature though. I'll suggest to kill processMultiTypes and go with something like:

on: function (types, callback, scope) { if (typeof types === "string") { types = types.trim().split(EventBusClass.DEFAULT.EVENT_TYPE_SPLIT_EXP) } types.forEach(function(type) { // move all the other code here }); }

We are making a few assumptions and we don't bother covering every single case:

The function on accepts a string with comma separated types or an array of types

We are not re-calling the on function we just assume that we are processing many types inside

Yes, perhaps this increases the readability of the code. ProcessMultiTypes method is to increase the unity of the EventBusClass.on and EventBusClass.off method to deal with type parameters, support for arrays or by comma and space separated string. If the return value equals EventBusClass object description has been processed, if no processing returns a single type processed by subsequent code.

krasimir · 2016-12-17T17:38:49Z

src/EventBus.js

+         * @param scope
+         * @returns {EventBusClass}
+         */
+        once: function (type, callback, scope) {


That's a nice one!

krasimir · 2016-12-17T17:43:00Z

src/EventBus.js

+            if (!isRegExpType) {
+                if (typeof this.listeners[eventType] != "undefined") {
+                    if (!(allCallback && allScope))
+                        iterator(this.listeners[eventType], function (i, listener) {


I feel that the iterator idea is kinda similar to forEach. Do you think that we can use it instead? It's just easier to follow the code.

Add iterator function like jQuery.each method, We can use it to traverse array elements and object attributes.

krasimir · 2016-12-17T17:43:25Z

src/EventBus.js

+                }
+            } else {
+                iterator(this.regexListeners, function (index, listener) {
+                    if (!(listener.eventType == eventType && isRemove(listener))) newArray.push(listener);


We should really use tripple = everywhere.

krasimir · 2016-12-17T17:46:52Z

src/EventBus.js

+            //  origin:'click',
+            //  endpoint:'onClick'
+            // })
+            if (arguments.length == 1 && typeof origin == "object") {


I tend to avoid such logic in my libraries. It's not like I'm not suing it though. But in the end it always makes the code complex. Treating the arguments of a function in different way based on their amount is kinda misleading for the developers that contribute to the project. Sounds like the function is doing more then one thing. Can you think of something like

if (arguments.length === 1 && typeof origin == "object")) { this._redirectObject(...) }

So we have single-job functions with clear signature.

Well, support a unified style of writing, in order to avoid confusion when used.

krasimir · 2016-12-17T19:53:01Z

src/EventBus.js

+         * @param processor processor be called before event redirection
+         * @return {EventBusClass}
+         */
+        redirect: function (origin, endpoint, condition, processor) {


I'm not sure that I understand the purpose of redirect. Can you please come with your use case.

Here we have again a method that does several things. I wish to have simpler methods that do only one thing.

The purpose of implementing the redirect method is to increase the renaming of the event so that the specified event is turned into other events, or a class event turns into a single event.

Specific use cases have been included in the example/node/cs-test.js.

krasimir · 2016-12-17T19:54:16Z

src/EventBus.js

+                });
+
+                if (result)return true;
+            } else if (isRegExpType) {


Maybe we should think about handling the regexp type of events in the same fashion as the other events. Otherwise we end up having those if (regex) { ... } else { ... }. And it seems that we'll have such statements all over the library.

It's a good idea for me to try.

support quick and intuitive configuration of event flow

# Conflicts: # example/node/flow-test.js # lib/eventbus.min.js # src/EventBus.js

tony_shen added 4 commits December 13, 2016 18:14

support regex for event type

1818759

support multi event types for listener

compatibility with older versions

73f1a18

repair EventBus.once lose any parameters.

b38d6d1

Event object add getArg function.

add Producer/Consumer use case

1a049f8

tony_shen added 3 commits December 16, 2016 22:46

modify .gitignore

bcb81d1

add EventBus.redirect feature

dc0b4a7

EventBus.redirect feature support pre-processor

c983f7d

krasimir requested changes Dec 17, 2016

View reviewed changes

tony_shen and others added 15 commits December 19, 2016 11:17

add EventBus.redirect use-case

8014413

event flow use-case

fc13c64

add event flow object property

a81143e

add EventBus.flow method

c9cee83

support quick and intuitive configuration of event flow

add isStop method for Event Object

1b18304

add flow & event object usage

4d4f396

add getEventIdsPath to Event Object

ce688f0

add plugin method

1ab14d4

add aspect method

67d2261

add aspect ,before,after,around method

c1eeca1

Merge branch 'develop'

ab66a85

# Conflicts: # example/node/flow-test.js # lib/eventbus.min.js # src/EventBus.js

modified aspect method

56f7ca7

modified aspect method

35c93da

Merge branch 'develop'

f65c3ed

update README.md

5e860b1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support regex for event type #9

support regex for event type #9

bonashen commented Dec 13, 2016

krasimir commented Dec 14, 2016

bonashen commented Dec 17, 2016

krasimir left a comment

krasimir Dec 17, 2016

bonashen Dec 19, 2016

krasimir Dec 17, 2016

krasimir Dec 17, 2016

krasimir Dec 17, 2016

bonashen Dec 19, 2016

krasimir Dec 17, 2016

krasimir Dec 17, 2016

bonashen Dec 19, 2016

krasimir Dec 17, 2016

krasimir Dec 17, 2016

bonashen Dec 19, 2016

krasimir Dec 17, 2016

bonashen Dec 19, 2016

krasimir Dec 17, 2016

bonashen Dec 19, 2016

support regex for event type #9

Are you sure you want to change the base?

support regex for event type #9

Conversation

bonashen commented Dec 13, 2016

krasimir commented Dec 14, 2016

bonashen commented Dec 17, 2016

krasimir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment