RFC: New directory structure for src/features/

[AprilSylph]

Right now, the structure for the features directory is:

features/
├── _index.json           Index of feature names
├── *.css                 Feature stylesheets
├── *.js                  Feature scripts
├── *.json                Feature manifests
├── */                    Additional feature files, e.g.:
│   ├── options/          Custom preferences interface files
│   └── script.js         Child scripts

Two things bother me about this.

1. Inconsistency: Most features don't have additional files, and thus do not have dedicated subdirectories. The ones that do have their own subdirectories don't even have all their code contained in those subdirectories, in order to comply with the expected structure of non-additional-file-having features.
2. Directory size: Because every feature consists of a minimum of two files (with many, I think the majority, consisting of three), a list of the directory's contents is artificially large. The repetition also hurts readability; when feature names start with the same substring, it can sometimes be difficult to parse when one feature's files end and another begins. I find that because of the repetition, my brain starts automatically de-duplicating the files as I read them in order to find the next feature's files; when I skim over timeformat's files, I can end up missing that timestamps is even there, because they're similar-looking strings when not paying full attention.

Thus, I would like to propose an overhaul of these files to fit a new, non-repetitive structure:

features/
├── index.json		Index of feature names
├── */			Feature folders
│   ├── index.css	Feature stylesheet
│   ├── index.js	Feature script
│   ├── index.json	Feature manifest
│   ├── options.css	Custom preferences stylesheet
│   ├── options.html	Custom preferences webpage
│   ├── options.css	Custom preferences script
│   └── *.js		Child scripts

The main drawback of implementing this would, of course, be that all currently-open pull requests making feature modifications would become conflicting and need rebasing. This would create a significant volume of work for @marcustyphoon, so I will consider their buy-in a requirement for doing this.

I'm also not 100% certain if flattening the options.* files into the feature directory proper is the right move. We don't currently have any features that utilise both custom preferences and child scripts, so there wouldn't be any mixing of those done in this move, but it would open the possibility of putting them flat in the same directory in the future. One could also argue that if we're grouping feature runtime files into subdirectories, we should keep the same treatment for feature preference interfaces as well? I could be persuaded in either direction.

What do you think?

[AprilSylph]

To be honest, I'm also not sure about src/features/index.json and src/features/<feature_name>/index.json both existing.

I feel like having both src/manifest.json and src/features/<feature_name>/manifest.json is probably worse, though, given the possibility of confusing systems trying to lint or otherwise scan the addon's files.

src/features/<feature_name>/feature.json could work, I guess?

[marcustyphoon]

Yeah, agreed. I dislike both index (literally: list of entries, e.g. in a book) and manifest (literally: what is included and where, e.g. onboard a ship) here. Sure, index.html has become the standard default file name for a path in the web space, but that implies that the folder contains other stuff and the index page is how you list and navigate to that stuff. Admittedly that's true of Tweaks, etc.

feature.json is consistent with package.json ("the folder containing this file is a [filename] and this is the metadata for it"), which is decent. Maybe meta.json or metadata.json could be options? _.json is probably a bad idea :P

[AprilSylph]

I like your reasoning, but thinking it out more, I feel like maybe index.json is the way to go, because if we instead name it something that actually makes sense, it feels like we don't have an excuse to also do that for the other files? But I don't feel like any name other than index, in the web development space, accurately conveys "this file doesn't really have a name, it's just the main file of its type for this directory", which is what I'm trying to go for.

Basically: I want a standard feature to be a named folder with two to three essentially unnamed files, rather than folderless named files (as currently) or a named folder with named files (counting any alternatives to index we come up with as names, because they would be framework-specific and not understood as "not names" in the same way by the average potential new contributor).

[AprilSylph]

Eh but then again package.json is a well-established way of doing metadata and we don't rely on any of the metadata at actual runtime (right...? I forgot) so maybe the exception is warranted actually.

[marcustyphoon]

I mostly agree. I'm trying to think about it from a new-contributor lens too, but I have conflicting thoughts of a) I have worked on no production codebases and my guesses about what an industry vet would find familiar are bad, b) I wonder if, if anything, an industry vet might actually be slightly more confused by common names like index.js when the file in question isn't a "barrel" file as index.js often is, and c) actually do I think we're optimizing more for industry-vet contributors or indie/new-to-js contributors, because I think we've only gotten the second one.

My current thought is that feature.js feature.json feature.css options.html options.js options.css is probably the most clear.

(Of course, more than anything, I think whatever appeals to you most personally should win, because the codebase being organized in a satisfying/practical way probably in the end will pay more dividends from your perspective than any other; working on things you like is nice and maybe motivating, I think the effect of this particular decision on new contributors will be minimal, and I don't have much of a preference and will work on this exactly as much either way :D)

[marcustyphoon]

My immediate thought is that one drawback of doing exactly what's stated in the issue is that tabs in an IDE often only show the file name, so I would give some thought to changing the directory structure but keeping the js/json file names. This is, though, a) kind of odd, and b) wouldn't be consistent for options files unless also making them e.g. *_options.*, and I know how much you love consistency. If we did do that, of course, it would resolve your comment.

The main drawback of implementing this would, of course, be that all currently-open pull requests making feature modifications would become conflicting and need rebasing. This would create a significant volume of work for @marcustyphoon, so I will consider their buy-in a requirement for doing this.

As long as git treats them as file renames and updated imports, I wouldn't worry too much about it. My guess is that would happen even if we squash merged a PR that did this. If necessary we could squash the PR down to "rename files commit; update imports commit" and then merge commit that, if we were careful. (I do this on my other project when I convert js files to ts, as I believe the diff can be big enough to confuse git in that case.)

[AprilSylph]

In VS Code, open files with identical filenames gain a differentiating directory slug in the tab just to prevent that confusion! Hopefully others do, too... but also I don't think working on two different features at the same time is too common a thing in the first place? Not to mention a questionable practice; even if two or more features need their code changed in the same PR, those changes should be one commit on the PR branch per changed feature, no? :P

[marcustyphoon]

In VS Code, open files with identical filenames gain a differentiating directory slug in the tab just to prevent that confusion!

They do indeed! I find it significantly slower to scan, though (kind of a similar statement to the one in the original post :D) I don't really care, though, ultimately.

even if two or more features need their code changed in the same PR, those changes should be one commit on the PR branch per changed feature, no? :P

I find this being a relevant assertion a fascinating potential look into your workflow! If you were making a util change that affected multiple features, would you not generally have an open VS Code tab for each affected file?

[AprilSylph]

Hmm, I guess I would say that those kinds of sweeping util changes should be avoided if possible...? For example, I really want to change how dom() works (the middle two arguments can be combined into one by just... checking if the object value is a function or not when attaching everything...) but it would touch too many features for the effort to retest every affected feature (and the risk that there's some bug missed in those retests) to make sense given the payoff of slightly better code. In my mind, those kinds of sweeping changes are unfeasible until they're strictly necessary, e.g. for upgrading to Manifest V3.

_{idk maybe my brain has been poisoned by ROI-obsessed work culture lol}

[marcustyphoon]

Hm—it seems to me like there's a big mental distinction for you between localized and wide-reaching changes, and I don't think I have that!

#1802 is an interesting example: as we discussed, I understand the appeal of having the finalized versions of the changes that get submitted for review be split up in a certain way. I certainly didn't develop them with that categorization front-of- mind, though. Perhaps my pattern is to focus on cohesiveness and interoperability by implementing all (or as many as possible) of the changes to create the desired functionality at the same time—with all of the relevant files open side-by-side, looking at the git diff as a collective—and then later focusing on rigor when cherry-picking/interactive staging the commits for submission.

I was going to say something about speculating that I tuned my process like that over time to try to focus on making the code the way it should be rather than keeping remnants of the past, philosophically reflecting this project's split from XKit 7 and my pattern of touching as few lines of code as possible in that repo even when the results are obviously shit... but, no, I had this process back then too, I just implemented the good version and then made it worse but cleaner-diffing. So, never mind :D

[marcustyphoon]

Semi-relevant: a while ago I thought about how we could reduce the number of json files, but I don't remember the details. iirc inlining them into feature js (export const meta =) doesn't work because we have side effects in feature js (I have legitimately thought in my darker moments about making cssMap a global). Putting them all in a single file (json, theoretically, but I would add export default and make it js because somehow jsonc/json5 aren't browser native) seems like generally a bad idea.

[AprilSylph]

I've had similar thoughts, but I don't know how we would prevent util side-effects without doing even more custom framework stuff, which I feel would heighten the barrier of entry to new contributors, which we are already kinda hurting for.

[marcustyphoon]

Yeah, there's definitely no way. I see why xkit 7 did frontmatter in comments even though that's obviously terrible.

[marcustyphoon]

This would create a significant volume of work for @marcustyphoon, so I will consider their buy-in a requirement for doing this.

Eh, only took like an hour and a half of mostly hitting the same shortcut keys repeatedly. I only needed to do a force push to a stacked-pr branch on a different repository, checked out the wrong repository, checked out the wrong branch on the right repository, force pushed to an entirely unrelated PR branch, and had to recover the correct commit using git reflog once!