search

cucumber-attic / microcuke

Mimimal Cucumber Reference Implementation
56 stars 10 forks source link

Don't use EventEmitter #8

Open aslakhellesoy opened 8 years ago

aslakhellesoy commented 8 years ago

Although it's idiomatic Node.js JavaScript it's not present in many other languages. This makes it hard to port the code.

We should replace it with a custom implementation that is easier to port.

Ref https://github.com/cucumber/cucumber/issues/8#issuecomment-181520132

Zearin commented 8 years ago

For your refactoring pleasure, all files under lib/ and test/ that use EventEmitter:

Zearin commented 8 years ago

How do you want to refactor this? With a roll-your-own Observer class?

aslakhellesoy commented 8 years ago

Not sure yet - need to stare at the code for a while.

Zearin commented 8 years ago

This page contains a useful comparison of different Observer patterns (with pros and cons for each):

https://github.com/millermedeiros/js-signals/wiki/Comparison-between-different-Observer-Pattern-implementations

mattwynne commented 8 years ago

FWIW, the 2.0 version of Cucumber-Ruby uses a pipes-and-filters setup, where the test cases are fired out of the compiler through a set of filters and finally into the runner. I really like this design as it's stateless, and it feels very modular. In the case of Ruby those filters just have to conform to a duck type / interface with a couple of methods, and we plug them together using the Chain of Responsibility trick - I think @everzet does something similar in Behat too.

Zearin commented 8 years ago

FWIW, the 2.0 version of Cucumber-Ruby uses a pipes-and-filters setup, where the test cases are fired out of the compiler through a set of filters and finally into the runner. I really like this design as it's stateless, and it feels very modular.

I may be misunderstanding you, because I still don’t have a super-clear understanding of the architectural big picture—and even less so for the version 2 Cucumber/Gherkin projects. But…

Wasn’t a central goal of the 3.0 version to totally decouple Cucumber and Gherkin?

On one hand, I do like “pipeline” architectures (which, off the top of my head, sounds like the “set of filters” you describe). On the other hand, I want to absolutely certain that the loose-coupling goal is not compromised. I believe the loose coupling is of paramount importance for both the cross-language appeal of Cucumber and Gherkin, as well as their long-term refinement and maintenance.

Now that I’ve blabbered all that, I still don’t know if I understood you correctly. I invite your enlightenment. :)

mattwynne commented 8 years ago

OK, perhaps I should clarify what I mean by "compiler".

There's a compiler that goes through the gherkin and compiles it into test cases. In Ruby, we fire each of those test cases out down this chain of filters, and finally through a runner.

So Cucumber has a dependency on the test case (or pickles as they've been named in Aslak's latest code). The interface for a filter is three methods:

# example of a pass-through (noop) filter

def with_receiver(receiver)
  @receiver = receiver
end

def test_case(test_case)
  @receiver.test_case(test_case)
end

def done
  @receiver.done
end

So the dependency is on this more abstract type, the test case, rather than on gherkin.

aslakhellesoy commented 8 years ago

There is an important distinction to make here. There are two different compiler implementations.

There is a compiler in cucumber-ruby which produces test cases from the AST. This design has been refined and reimplemented in gherkin's own compiler (and ported to other languages). This compiler produces pickles.

Cucumber-Ruby doesn't (yet) use gherkin's compiler - it still uses its own.

The goal is to have all Cucumber implementations use the gherkin compiler:

Gherkin Doc--[Gherkin Parser]-->AST--[Gherkin Compiler]-->Pickles--[Cucumber]-->test cases.

This is how we'll move towards a more uniform design across platforms.

Zearin commented 8 years ago

For your convenience, here is an expanded reference of the events:

Summary

The only events are:

Recommendation

Microcuke is designed to be minimalist and loosely-coupled. Based on the descriptions from the Observer patterns comparison page I listed earlier, I think we should go with the Publish/Subscribe implementation pattern.

I propose we put a simple Pub/Sub “manager” in its own module. Despite the name of the “Publish/Subscribe” pattern, it still fundamentally deals with “events”; therefore, I propose we name the new module events, because I think that name carries the greatest recognition and comfort level for people interested in using microcuke as a reference implementation.

@aslakhellesoy, If you agree with this, I can write up something small and quick for a Pull Request. (If not, please tell me the other considerations or concerns about factoring out EventEmitter.)

Zearin commented 8 years ago

@aslakhellesoy ping :)

aslakhellesoy commented 8 years ago

busy!

Zearin commented 8 years ago

k!