search

optimizely / nuclear-js

Reactive Flux built with ImmutableJS data structures. Framework agnostic.
https://optimizely.github.io/nuclear-js/
MIT License
2.23k stars 141 forks source link

grouping observers by getters in addObserver #183

Open lyonlai opened 9 years ago

lyonlai commented 9 years ago

This PR is a better implementation of the grouping observers by getters idea in

https://github.com/optimizely/nuclear-js/pull/182

The difference is observers are grouped when they are added. Also the removal of observers has been taken care of too.

jordangarcia commented 9 years ago

@lyonlai I checked out your branch and couldn't get tests to pass using grunt karma:chrome and running them in debug mode.

I had initially tried an approach where the observerMap was using a getter by reference as a key, however I couldn't get it it to pass tests.

If you are able to get tests to pass I'm happy to include.

lyonlai commented 9 years ago

@jordangarcia I'll get back to you soon on this.

lyonlai commented 9 years ago

@jordangarcia looks like I've got some missing files when I tried to run the test. after I've install a bunch of missing npm packages now I've got "Cannot find module './common'"

jordangarcia commented 9 years ago

@lyonlai hm, ill try cloning a fresh repo and see if anything is missing

lyonlai commented 9 years ago

@jordangarcia while you are doing that I'll run karma manually to see if I can get the test running.

jordangarcia commented 9 years ago

@lyonlai it's working for me, not sure if it's becuase I have a globally installed module that maybe you don't have.

Do you have karma install globally?

Are you running on OSX?

lyonlai commented 9 years ago

@jordangarcia I don't have things globally. I think that's why. I'm on osx. btw what node version you are running. and also npm version?

jordangarcia commented 9 years ago

I'm running node v0.12.5 (npm v2.11.2)

lyonlai commented 9 years ago

looks like the version problem. Was using node 4. karma running after change. I'll dig into the test now.

lyonlai commented 9 years ago

@jordangarcia I've fixed the failing tests, added two more tests about the getters map and fixed the formatting issue.

jordangarcia commented 9 years ago

I'm going to release 1.2 first since i've done more extensive testing using Optimizely's integration tests.

After 1.2 is released I will merge this and put in 1.2.1

lyonlai commented 9 years ago

sure Thanks @jordangarcia .