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

Unregistering component callbacks from Stream(Rules)Store. #3378

Merged
merged 3 commits into from Jan 18, 2017

Conversation

Projects
None yet
2 participants
@dennisoelkers
Member

dennisoelkers commented Jan 17, 2017

Description

Motivation and Context

Before this change, the StreamComponent was registering callbacks to be
notified when the Stream(Rules)Store state changes. Unfortunately, there
was no way to unregister these callbacks, so after unmounting the
component its callbacks were still called.

After this change, there is now a way to unregister callbacks which is
consumed in the componentWillUnmount function of the StreamComponent.

Fixes #3374.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@dennisoelkers dennisoelkers added this to the 2.2.0 milestone Jan 17, 2017

@edmundoa edmundoa self-assigned this Jan 17, 2017

@edmundoa

Apart from the usage of lodash, the change fixes the issue with StreamComponent.

While testing the changes, I saw that we also register callbacks for StreamsStore and StreamRulesStore in StreamRulesEditor, and we should also ensure to unregister in that component (it is trying to update the state of unmounted components).

@@ -75,6 +76,9 @@ class StreamRulesStore {
_emitChange() {
this.callbacks.forEach((callback) => callback());
}
unregister(callback: Callback) {
lodash.pull(this.callbacks, callback);

This comment has been minimized.

@edmundoa

edmundoa Jan 17, 2017

Member

I know we use lodash in one or two places already, but I honestly see no benefit extending its use for things like this. IMHO using filter from the standard library is simple enough, does not require anybody to learn anything else, and does not require the external dependency.

This comment has been minimized.

@dennisoelkers

dennisoelkers Jan 17, 2017

Member

Using filter it would be this.callbacks = this.callbacks.filter((c) => c !== callback) which is harder to understand. As lodash also offers a lot of other useful functions that are missing in the javascript standard library, I think it's worth the (maximum) 24kb of overhead.

This comment has been minimized.

@edmundoa

edmundoa Jan 17, 2017

Member

I don't doubt lodash is useful, but so far we only use a single function of it. As far as I know, nobody in the team knows it and we don't even have it as a direct dependency.
I'm not against adding 24kb of dependency if we need it (we already have many others, so there's not much difference), but in this case I have the feeling we are using it for some small eye candy.

This comment has been minimized.

@dennisoelkers

dennisoelkers Jan 17, 2017

Member

The fact that it is not an explicit dependency yet is a problem, agreed. We use it in another place already, so this change is not introducing it in the first place and I think both future and existing code could benefit from it, once we are using it. Documentation is excellent too, so there is no real barrier for people in the team not knowing it yet.

This comment has been minimized.

@edmundoa

edmundoa Jan 18, 2017

Member

Then please introduce the explicit dependency.

To be honest I rather stick to the standard library whenever possible, specially when there is an equivalent for the utility functions introduced in other libraries like lodash or jQuery, but we don't have a rule for that.

In my opinion the addition of lodash to the toolchain is bearable for the current team, but we will face challenges (and to be fair not just caused by lodash) when new team members arrive. The amount of technologies to learn in order to write some frontend code is really huge. We are not alone at this, as many node and JS projects have the same issue, but I think without a careful selection of the libraries we use, the problem will be even worse.

This comment has been minimized.

@dennisoelkers

dennisoelkers Jan 18, 2017

Member

I really appreciate the thought you are putting into this, thank you. I have a differing opinion towards lodash though, because it's not like react/reflux something you need to learn to be able to write code, but it is something you can utilize when you see the need for it. So it does not raise the barrier for new team members to be productive. Instead, it allows to write more concise and succinct code when you are ready.

Long story short, I am gonna add the explicit dependency and I hope you are alright with that.

@@ -149,6 +150,9 @@ class StreamsStore {
_emitChange() {
this.callbacks.forEach((callback) => callback());
}
unregister(callback: Callback) {
lodash.pull(this.callbacks, callback);

This comment has been minimized.

@edmundoa

edmundoa Jan 17, 2017

Member

Same comment concerning lodash as before.

dennisoelkers added some commits Jan 17, 2017

Unregistering component callbacks from Stream(RulesStore).
Before this change, the StreamComponent was registering callbacks to be
notified when the Stream(Rules)Store state changes. Unfortunately, there
was no way to unregister these callbacks, so after unmounting the
component its callbacks were still called.

After this change, there is now a way to unregister callbacks which is
consumed in the componentWillUnmount function of the StreamComponent.

Fixes #3374.
Explicitly adding lodash as dependency of the web interface.
It was missing before, although used in at least one line of code. The
reason why it worked before is because it was drawn it transitively
through other libraries depending on it.

@dennisoelkers dennisoelkers force-pushed the issue-3374 branch from eab169d to 8209e66 Jan 18, 2017

@edmundoa

LGTM 👍

@edmundoa edmundoa merged commit af335a3 into master Jan 18, 2017

2 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
ci-web-linter Jenkins build graylog-pr-linter-check 1302 has succeeded
Details
licence/cla Contributor License Agreement is signed.
Details

@edmundoa edmundoa deleted the issue-3374 branch Jan 18, 2017

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