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

Possibly structure background code a bit more. #129

Closed
creesch opened this issue Aug 13, 2019 · 3 comments · Fixed by #171
Closed

Possibly structure background code a bit more. #129

creesch opened this issue Aug 13, 2019 · 3 comments · Fixed by #171

Comments

@creesch
Copy link
Member

creesch commented Aug 13, 2019

I am not sure it is needed (yet), but we have been putting more stuff in background.js and it might be worth it to structure it a bit more and possibly split it up according to functionality.

Just making this issue so we can discuss it and so I don't forget.

@eritbh
Copy link
Member

eritbh commented Oct 4, 2019

I'd argue that this is probably needed sooner than later. Especially with notifications, web requests, and other stuff becoming more complicated, this is something we should tackle sooner than later - don't want to leave it too long and have to refactor some truly massive file later. I have some ideas for this that I can write up in a bit.

@creesch
Copy link
Member Author

creesch commented Oct 4, 2019

Yeah sounds good, at the very least we should indeed brainstorm about it instead of letting it get stale here.

@eritbh
Copy link
Member

eritbh commented Oct 4, 2019

I think we should revise the format we use for message passing to the background page, and then we can use a separate file for each type of message that comes in. e.g. tb-request is separated from tb-global is separated from tb-notification. We could even tackle some of #44 at the same time.

// manifest.json
{
	// ...
	"background": {
		"scripts": [
			"data/libs/jquery-3.4.1.min.js",
			"data/background/background.js",
			"data/background/actions/one.js",
			"data/background/actions/two.js"
		],
		"persistent": true
	},
	// ...
}
// background/background.js
const actionHandlers = {};
browser.runtime.onMessage.addListener(([action, options], sender) => {
	if (!actionHandlers[action]) {
		log.error('Received unknown action:', action, options);
		return;
	}
	return actionHandlers[action](options, sender);
}
// background/actions/one.js
actionHandlers.one = (options, sender) => {
	// A simple synchronous message handler
	return 'some value'; // The return value is sent as the message response 
}
// background/actions/two.js
async function someStuff () {
	// ...
}
actionHandlers.two = async (options, sender) => {
	// A simple asynchronous message handler
	const response = await someStuff();
	return response; // The return value is still sent as the message response
}

This would change the pattern for sending messages from sendMessage({action, ...otherOptions}) to sendMessage([action, otherOptions]) which is good because then we can use the action key as an option rather than it needing to be special (tb-global would benefit from this). This also means that each file can maintain its own utility functions without getting too messy.

@eritbh eritbh mentioned this issue Oct 14, 2019
4 tasks
@eritbh eritbh self-assigned this Oct 15, 2019
@eritbh eritbh modified the milestone: v5.3 Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants