Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

grouping observers by getters in addObserver #183

Open
wants to merge 9 commits into
base: jordan/refactor-everything!
Choose a base branch
from

Conversation

lyonlai
Copy link

@lyonlai lyonlai commented Nov 1, 2015

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

#182

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

@@ -171,18 +171,26 @@ exports.addObserver = function(observerState, getter, handler) {
handler: handler,
})

let updatedObserverState
let updatedObserverState = observerState.updateIn(['gettersMap', getter]
Copy link
Contributor

Choose a reason for hiding this comment

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

please use the style:

let updatedObserverState = observerState.updateIn(['gettersMap', getter], observerIds => {
  return observerIds ? observerIds.add(currId) : Immutable.Set.of(currId)
})

@jordangarcia
Copy link
Contributor

@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
Copy link
Author

lyonlai commented Nov 1, 2015

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

@lyonlai
Copy link
Author

lyonlai commented Nov 1, 2015

@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
Copy link
Contributor

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

@lyonlai
Copy link
Author

lyonlai commented Nov 1, 2015

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

@jordangarcia
Copy link
Contributor

@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
Copy link
Author

lyonlai commented Nov 1, 2015

@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
Copy link
Contributor

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

@lyonlai
Copy link
Author

lyonlai commented Nov 1, 2015

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

@lyonlai
Copy link
Author

lyonlai commented Nov 1, 2015

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

map.update('any', getters => getters.remove(getter));
} else {
storeDeps.forEach(storeId => {
map.updateIn(['stores', storeId]
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think this is correct.

If we observe add two observer entries with the same getter and then unobserve one it will unobserve all entries with that getter.

Copy link
Author

Choose a reason for hiding this comment

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

that's why it's under the condition, only removed when there's no entries listening to that getter

if (map.getIn(['gettersMap', getter]).size <= 0) 

Copy link
Author

Choose a reason for hiding this comment

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

I can add a test case to cover it if you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! This looks good.

… still handler for the getter, do not remove the reference in store.
@jordangarcia
Copy link
Contributor

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

@jordangarcia jordangarcia added this to the 1.2.1 milestone Nov 2, 2015
@lyonlai
Copy link
Author

lyonlai commented Nov 2, 2015

sure Thanks @jordangarcia .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants