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

Included sender information in action information #41

Merged
merged 2 commits into from
Dec 29, 2016
Merged

Included sender information in action information #41

merged 2 commits into from
Dec 29, 2016

Conversation

durandj
Copy link
Contributor

@durandj durandj commented Dec 27, 2016

It can be helpful to know about who sent the message/action. An example
of why this might be helpful is when using a content script to perform
something tab specific. Without the tab information you won't be able
to get the Tab ID or other useful information about where the action
came from.

The exact use case I have is I'm creating a Chrome extension that has
a page action. The page action should only be available based on the
availability of a script being on the page (it's a work thing). Chrome
allows for this on a per tab basis but you need the tab ID to be able
to do this. My content script can't access this information except by
sending a message. I'm already sending a message when I dispatch
an action. It would be helpful to get this information from the action
on the background side of the extension.

It can be helpful to know about who sent the message/action. An example
of why this might be helpful is when using a content script to perform
something tab specific. Without the tab information you won't be able
to get the Tab ID or other useful information about where the action
came from.
@vhmth
Copy link
Collaborator

vhmth commented Dec 28, 2016

For each of my content scripts, I do the same dance and request the tabID from the background script via message passing and then pass it down as a prop through my entire app.

Even though this kinda takes a step outside of the concerns of this library, I think it's enough of a common use case in developing a Chrome extension to be added. Enhancing actions also seems to be the way to do it.

Thoughts @tshaddix?

@tshaddix
Copy link
Owner

tshaddix commented Dec 29, 2016

I think this is a great idea @durandj, and I've run into the same problem myself. I even started a branch and started working on this addition a few months ago. I'm personally not against the API change proposed in this thread. @vhmth what are your thoughts on the proposed API change? The public API doesn't change really, but the action side effect does (adding a _sender field).

@durandj Thanks for working on this!

@durandj
Copy link
Contributor Author

durandj commented Dec 29, 2016

@tshaddix, what are your thoughts on a way to do this without a side effect? I agree that a side effect like this one isn't ideal but it was simpler to implement without a ton of changes and should be relatively transparent to the end user.

@tshaddix
Copy link
Owner

@durandj Sorry if I wasn't clear in my response.

I like your solution quite a bit. I just wanted a second opinion before moving forward.

@durandj
Copy link
Contributor Author

durandj commented Dec 29, 2016

No problem just making sure :)

@tshaddix
Copy link
Owner

tshaddix commented Dec 29, 2016

@durandj To answer your question: I really don't see a good automatic way to do this without some side effect. We could include some interface/handler that allows message interception before dispatch and include some implementation of this interface that includes the sender, but that seems like overkill for the problem we are trying to solve.

I think your solution does a good job of being simple and complete while making use of existing APIs and expectations. We should make sure to update the documentation in the README when we move forward.

:)

Copy link
Owner

@tshaddix tshaddix left a comment

Choose a reason for hiding this comment

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

This looks good to me.

Copy link
Owner

@tshaddix tshaddix left a comment

Choose a reason for hiding this comment

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

Little code change.

@@ -44,7 +44,11 @@ export default (store, {
*/
chrome.runtime.onMessage.addListener((request, sender, sendResponse) => {
if (request.type === DISPATCH_TYPE) {
dispatchResponder(store.dispatch(request.payload), sendResponse);
const action = Object.assign({}, request.payload, {
_sender: sender,
Copy link
Owner

Choose a reason for hiding this comment

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

Can we remove the trailing comma here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure no problem! Force of habit from my works style guide.

Copy link
Owner

Choose a reason for hiding this comment

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

I hear you! I'm not against it in principle! Just want to stay consistent within the package :)

Copy link
Owner

@tshaddix tshaddix left a comment

Choose a reason for hiding this comment

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

Awesome stuff!

@vhmth
Copy link
Collaborator

vhmth commented Dec 29, 2016

Yep I'm 100% cool with this. We can add to the README when we bump the version @tshaddix

@tshaddix tshaddix merged commit 9901fd6 into tshaddix:master Dec 29, 2016
@tshaddix
Copy link
Owner

@vhmth Awesome, thanks!

@durandj @vhmth We are all merged and ready to go. Created issue to track README update and version bump: #43

@vhmth
Copy link
Collaborator

vhmth commented Dec 29, 2016

👍

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.

None yet

3 participants