Skip to content
This repository was archived by the owner on May 6, 2025. It is now read-only.

Conversation

@UDenis
Copy link

@UDenis UDenis commented Oct 31, 2014

Будет время гляньте и откоментируйте

@UDenis
Copy link
Author

UDenis commented Oct 31, 2014

ping @sergeche , @a-ignatov-parc

index.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Без скобок для for адово уже

@a-ignatov-parc
Copy link
Contributor

Ну и было бы не плохо вот это сделать #5

@UDenis
Copy link
Author

UDenis commented Oct 31, 2014

ок

@sergeche
Copy link
Contributor

Тут опять вижу проблемы с проектированием: зачем на запрос команды отвечать отдельным событием? Я себе логику работы представляю вот так:

eventBus.request('get-something').then(function() { ... });

// отвечаем на команды
eventBus.on('get-something', function(response) {
    doAjaxRequest('/my/endpoint', function(data) {
        response.resolve(data);
    });
});

То есть метод .request() всего лишь создаёт promise или callback-функцию (как в Node.JS, первый аргумент — ошибка) и отдаёт его всем подписчикам. А подписчики, в свою очередь, вызывают его, если могут обработать сообщение.

@UDenis
Copy link
Author

UDenis commented Oct 31, 2014

@sergeche , я тебя не понял
сейчас работать можно так

eventBus.request('get-something', arg1, arg2).then(function() { ... });

// несколько вариантов отвечать на команды response
eventBus.on('get-something', function(arg1, arg2) {
   // можно вернуть promise, явно на вызывая response
   return $.ajax('/my/endpoint');
});

// несколько вариантов отвечать на команды response
eventBus.on('get-something', function(arg1, arg2) {
   // можно простой результат
   return { ... };
});

// Если подписаться через subscribe
// то также работают описанных выше два способа
// + response
eventBus.subscribe('get-something', function(data, env) {
   doAjaxRequest('/my/endpoint', function(data) {
        env.reply(null, data); //resolve
        // or
        env.reply(err); //reject
    });
});


@sergeche
Copy link
Contributor

Мой вопрос: зачем весь этот зоопарк решений и переопределение функциональности postaljs, если вся задача сводится к созданию и передаче подписчикам управляющей функции/promise?

@sergeche
Copy link
Contributor

Вот смотри, представим себе чистое окружение, в котором кроме eventBus больше ничего нет. И для ответа на команду мне нужно сделать аякс-запрос. Ни в одной из твоих реализаций я не смогу сделать вот так:

eventBus.on('get-data', function(response) {
    var xhr = new XMLHttpRequest();
    xhr.open('GET', '/url');
    xhr.addEventListener('complete', function() { response.resolve(xhr.resonseText); });
    xhr.send();
});

Потому что мне нужно либо сразу вернуть ответ, либо вернуть deferred, причём, не любой deferred, а именно тот, который понимаешь ты. То есть тянуть за собой jQuery, котороя является неявной зависимостью этой библиотеки.

@UDenis
Copy link
Author

UDenis commented Oct 31, 2014

В таком случае, можно просто оставить так

eventBus.request('get-something', arg1, arg2).then(function() { ... })

eventBus.subscribe('get-something', function(data, env) {
   doAjaxRequest('/my/endpoint', function(data) {
        env.reply(null, data); //resolve
        // or
        env.reply(err); //reject
    });
});

и забить на eventBus.on, либо сделать так

// несколько вариантов отвечать на команды response
eventBus.on('get-something', function(reply, arg1, arg2) {
   // можно простой результат
   reply(null, data); //resolve
   // or
   reply(err); //reject
});

@sergeche
Copy link
Contributor

Ещё раз повторю вопрос: зачем столько разных вариантов, которые работают/не работают в определённых окружениях, если можно оставить один, который будет работать везде?

@UDenis
Copy link
Author

UDenis commented Oct 31, 2014

мне казалось, что возвращать promise или просто объект, будет удобнее в использовании, вместо необходимости явно вызывать reply, возможно я ошибся.
Сделаю как ты предлагаешь

@sergeche
Copy link
Contributor

Так как promise не является (пока) чем-то нативным, это означает, что для работы с твоим механизмом нужна будет ещё одна зависимость, что гораздо неудобнее, чем вызвать переданный promise/функцию.

@UDenis
Copy link
Author

UDenis commented Oct 31, 2014

Серега, спасибо за комментарии, понял твою мысль, уберу лишнее.

@UDenis
Copy link
Author

UDenis commented Nov 5, 2014

Реализовал следующую интерфейс при работе с командами

// послать команду
bus.request('resolve', 2, 3).then(function(){});
// or
bus.publish({
    topic: '/resolve',
    data: [2, 3]
 }).then(function(){});

// обработать и вернуть ответ
bus.on('resolve', function(reply, arg1, arg2) {
    reply(null, 1 + arg1 + arg2) // or reply.resolve(1 + arg1 + arg2)
       //reply(err); or reply.reject(1 + arg1 + arg2)
});

bus.subscribe({
    topic: 'resolve',
    callback: function(data, env) {
        env.reply.resolve(1 + data.args[0] + data.args[1]);
    }
});

по поводу поддержки команд через метод trigger

bus.on('resolve', function(reply, arg1, arg2) {
    // reply вызывать нет смысла, т.к 
    // при вызове через trigger нет ожидающих ответа подписчиков
        // т.к нет возможности их указать
});

bus.trigger('/resolve', 2, 3);

@a-ignatov-parc
Copy link
Contributor

Для trigger нужно реализовать аналог метода then который позволит подписаться на результат выполнения иначе получается не консистентный функционал. Команда вроде как есть, но воспользоваться ей нельзя.

Грубый пример того как можно реализовать метод then без поломки чейнинга и общего интерфейса:

// Интерфейс фасада
{
    on: function() { ... },
    off: function() { ... },
    trigger: function() { ... },
    then: function(callback) {
        if (typeof callback === 'function') {
            callback();
        }
        return this
    }
}

// Как сделать чтоб `then` работал правильно при вызове команды через метод `trigger`
// [INFO] Это набросок реализации.
{
    trigger: function() {
        if (isCommand) {
            var deferred = $.Deferred();

            bus.publish({
                data: [deferred, ..args]
            });

            return _.extend({}, this, {
                then: deferred.then.bind(deferred)
            });
        }
    }
}

После чего если была вызвана команда то then будет завязываться на резолв промиса, если команда не была вызвана, то then будет сразу же выполнять колбек.

Оба варианта будут возвращать ссылку на интерфейс фасада.

@a-ignatov-parc
Copy link
Contributor

Так же мне никто не ответил зачем в посталовском паблише нужно передавать топик с префиксом / это кейворды для бекбоновского фасада, но не для постала

@UDenis
Copy link
Author

UDenis commented Nov 5, 2014

_.extend({}, this, {
                then: deferred.then.bind(deferred)
            });

мне не нравится это решение тем что у нас появится куча объектов клонов и мы просто
наплодим их в памяти. Мне кажется игра не стоит свеч ради сохранения чейнинга.
Чем обаснованно желание сохранить чейнинг и добавить then?
Если ты хочешь выполнить команду, вызываей request, а не триггер

По поводу / - как иначе указать что это команда?
В посталовском publish было два варианта указать что это команда
через headers, указав заголовок

bus.publish({
    topic: '/reject',
    data: 2,
   headers: {
     replyable: true
  })

Либо через /.
Я выбрал /, иначе было бы не понятно, почему в триггер команды вызываются через /

Аналогично, при вызове вызове событий, сохраняющих состояние (!). Знак ! используется как в триггер, так и в паблиш

@sergeche
Copy link
Contributor

sergeche commented Nov 5, 2014

Я вот честно не понимаю необходимости менять внутренности publish/subscribe для этой задачи. Почему не оставить один простой контракт вида bus.request('command').then() и bus.on(command, function(reply) {})? У меня лично вообще не возникает желания использовать для этих целей громоздкий синтаксис publish/subscribe.

@a-ignatov-parc
Copy link
Contributor

Тут больше не в чейнинге удобство, а в простой конструкции фасада. А по идее у него есть лишь базовые методы on, off, trigger и префиксы у событий для модификации поведения. А чейнинг это одно из условия консистетности поведения фасада.

Если добавляется другой метод для реализации функционала, то префиксы в нем уже не нужны. Проблемы с памятью это оч надуманный момент + GC все это подчистит когда объект перестанет использоваться. В первую очередь нужно думать об удобстве использования.

У постала все параметры передаются через хедеры, так зачем придумывать что-то свое чего нет в базовом функционале? Напомню, что префиксы придуманы для фасада для модификации поведения (считай это плагинами)

@UDenis
Copy link
Author

UDenis commented Nov 5, 2014

убрал переопределение метода publish
Оставил только это

// послать команду
bus.request('resolve', 2, 3).then(function(){});

// обработать и вернуть ответ
bus.on('resolve', function(reply, arg1, arg2) {
    reply(null, 1 + arg1 + arg2) // or reply.resolve(1 + arg1 + arg2)
       //reply(err); or reply.reject(1 + arg1 + arg2)
});

bus.subscribe({
    topic: 'resolve',
    callback: function(data, env) {
        env.reply.resolve(1 + data.args[0] + data.args[1]);
    }
});

Антон, поддержку then для триггер, не делал, пока не уверен что нужно.
Но думаю, что можно сделать как-то так:

bus.trigger('@chanel', '/command', arg1, arg2, function(err, data){  });

т.е передавать callback всегда последним аргументом

@sergeche
Copy link
Contributor

sergeche commented Nov 5, 2014

Зачем вообще делать trigger? Почему не bus.request('@channel', 'command', args..).then()?

@UDenis
Copy link
Author

UDenis commented Nov 5, 2014

сейчас как раз есть возмoжность выполнить такое

 bus.request('@channel', 'command', args..).then()

@sergeche
Copy link
Contributor

sergeche commented Nov 5, 2014

Я понимаю, что есть такая возможность. Я не понимаю, зачем в принципе в контексте работы с request/response нужно делать trigger()?

@a-ignatov-parc
Copy link
Contributor

Для trigger все, что после первых двух параметров это аргументы для обработчика события. Делать точно не нужно. Вообще про триггер я заикнулся потому что забыл про отдельный метод request и в связи с витанием в воздухе префикса /, который по идее должен был использоваться лишь в тригере для модификации поведения.

Если оставляем только request и выпиливаем везде префикс /, то я ЗА

@UDenis
Copy link
Author

UDenis commented Nov 5, 2014

оставил только request и выпилил везде префикс /

@a-ignatov-parc
Copy link
Contributor

Отлично! И нужно написать тесты для request в тестах фасада

Copy link
Contributor

Choose a reason for hiding this comment

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

Оч удобно в комитах добавлять текст fix #5 что автоматически закроет issue #5 при мердже в мастер

@UDenis
Copy link
Author

UDenis commented Nov 5, 2014

Стоит ли прикрутить EventStreaming как в FRP (на примере Bacon.js)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Зачем использовать другую библиотеку промисов если:
а) это внутренний механизм для работы библиотеки
б) промисы по умолчанию встроены в твою библиотеку (то есть не получится их выпилить чтобы сэкономить на размере)

Copy link
Author

Choose a reason for hiding this comment

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

что значит использовать другую библиотеку?
Ты предлагаешь вообще отказаться от промисов или использовать свою реализацию ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Нет, это комментарий к твоему предыдущему, удалённому комментарию про возможность использования других промисов, если захочется

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants