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

No-editor, no-eval branch #2186

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

marcustyphoon
Copy link

@marcustyphoon marcustyphoon commented Jan 7, 2025

The XKit Editor, which lets users customize the code of their scripts from a special page in the browser, is built on the fact that New XKit stores its executable extension code in storage and runs it using eval. This is how user script extensions work, and as I understand it is tied to the history of this extension as a collection of user scripts and/or as a user script manager specifically for Tumblr.

This is kind of neat, and doesn't seem to break any Firefox addon policies per my line-by-line reading... and, I mean, violentmonkey is on the store, right? On the other hand, I have to assume that if we had metrics they would report that the editor functionality is basically completely unused by our userbase. The core functionality of the extension would still inarguably exist without the editor, and maybe a reviewer would argue that that's not a valid use of eval. And review aside, I think there are valid arguments to be made on both sides of whether the editor version or the no-editor version of the codebase is better. (It does kind of involve deciding on the purpose of this codebase going forward, which is another discussion.)

Anyway, I've never been interested in having that decision hinge entirely upon whether one version of the codebase exists and functions or not; I had the technical side solved ages ago. Thus, here is a fully functional branch with the XKit editor's save functionality disabled, with no eval use or storage of extension code and which thus has no version-number-based internal update functionality because the latest versions of scripts are always run directly out of the package (internal update notifications are gone, though we could put them back). This includes cleaning up all remaining web-ext lint warnings by factoring or killing some code in outdated scripts.

Discuss!

Technical notes:

  • XKit's internals expect script metadata to be accessible synchronously, but dynamic import (like extension storage access) is asynchronous. XKit Bridge makes storage contents synchronously accessible by keeping them in a global variable and delaying initialization while populating it to solve this; I had to create XKit.installed.data and XKit.installed.init() to serve a similar purpose.
  • install_extension() in xkit.js was left as-is in my 7.10.0 migration to keep the API surface of what used to be "the JSON downloaded from XKit servers/Github Pages" stable, but there's no point to doing that anymore and it's awful. It's implemented as tweaks to the defaults in loadExtensionData() here.

The rest of the diff should be comprehensible from the above points.

@marcustyphoon marcustyphoon force-pushed the no-eval branch 2 times, most recently from 6f5501d to 4d0b93e Compare January 26, 2025 04:48
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.

1 participant