Improve code accessibility

@ryansolid

I think that we can do a better job with making the code more accessible to developers who read the code for the first time as well as developers with disabilities and beginners.
We probably need to do some more research unless someone with more experience in this field wants to weigh in. However I already gathered the following points:

Add comments (obviously). This has been discussed before and is the plan but I thought that it belongs in this issue.
Reformat the whole codebase and limit line length to 79 or 78 characters. This should help people programming on braille displays and will probably also be more accessible to people coding on mobile. Some people can't afford a computer and I think that it's good to keep them in mind as well.
Prettier already defaults to 80 except for comments, so I would still suggest changing it to 79 or 78 to allow for git diffs to fit within 80 characters and maybe also leave room for one more character that some braille displays use to indicate that there's more text at the end of the current line. Other than that maybe comments don't have to be 79 / 78 characters since they can be wrapped, I'm not sure.
Make the code more self documenting. Currently IMO the code is not very self documenting, I personally struggled a lot with trying to read the code the first time, I felt like I was some archeologist trying to decipher an ancient language. It might be partly on me, but at least I feel like we shouldn't have one letter variables.
Break files into smaller files. The largest file right now is probably /packages/solid/src/reactive/signal.ts. This file is 1647 lines. I'm not sure if there is a consensus that reasonably sized files are more accessible, for me personally this makes the code significantly harder to read as it's easier for me to lose context and hard for me to look at a few functions at the same time if they are at separate parts of the file since I can't have them open in separate files. I know that @ryansolid and @rturnq prefer those bigger files as we discussed this before, but I do feel like this is less accessible.
Use tabs instead of spaces. Right now we are using 2 spaces and it seems like this can be inaccessible to people with vision impairment.
See: https://dev.to/alexandersandberg/why-we-should-default-to-tabs-instead-of-spaces-for-an-accessible-first-environment-101f

I'll try doing more research about this topic but we can start implementing those things for now. If anyone else wants to add ideas or comment on the ideas I presented here that would be very helpful!

Some random ideas:

Have mostly one file per exported function: having many many functions in a single file makes it harder to find the one you are looking for and to see all the exported functions at a glance.
Fight the type system less: there are too many type assertions IMO, that means you kind of have to rely on hidden assumptions to understand what's going on, and those assumptions may or may not be obvious, rather than relying on what the type system tells you.
Improve discoverability and code navigability of solid/solid & ryansolid/dom-expressions: currently most of the important code is split between two completely separate repositories under different users/organizations, and code in those repositories often imports things that don't exist because some imports are rewritten at build time, that makes it hard to discover where things are defined and to jump from thing to thing.
Some code is not particularly idiomatic, like assignments inside if should never happen IMO, those kind of things should probably get rewritten: e.g. if ((result = item != null && item[$RAW]))

Some more:

Have all the exports at the end of files, rather than at random rows in files, otherwise one needs to scan the entire file to find all the exports. Plus some exported functions are found between non-exported functions, that also makes it harder to find what functions are named because their names won't be found at a single predictable column index.
Define all types and interfaces in files containing only types and interfaces, that makes for cleaner separation of concerns in my opinion.
Use type instead of interface for things that won't need to be extended through interface merging, currently the two seem to be used somewhat interchangeably.

Thanks for adding more suggestions:

Fight the type system less: there are too many type assertions IMO, that means you kind of have to rely on hidden assumptions to understand what's going on, and those assumptions may or may not be obvious, rather than relying on what the type system tells you.

I also noticed that. It also makes the code more verbose at times because we have to do this type assertion on every reference. What do you propose to do in order to fix it? I think that the only fix is type guards, won't that slightly affect performance and bundle size though?

I also noticed that. It also makes the code more verbose at times because we have to do this type assertion on every reference. What do you propose to do in order to fix it? I think that the only fix is type guards, won't that slightly affect performance and bundle size though?

I wouldn't expect any measurable performance impact out of writing these things more cleanly, the bundle size may be larger but personally I wouldn't trade significantly cleaner code for marginally smaller bundles sizes.

For non-null assertions we can probably just wrap the thing in an if, i.e. either if ( !Owner ) return; or if ( Owner ) { /* previous code */ }. For as assertions the solution might be different in different scenarios, generally a typeof or an instanceof check costs basically nothing anyway.

For uncompressed bundle sizes, which at the end of the day are reflective of the actual code that will need to be parsed by the browser, no amount of trickery like that will beat mangling the library, so if that's a major goal mangling should probably be enabled instead.

the bundle size may be larger but personally I wouldn't trade significantly cleaner code for marginally smaller bundles sizes.

I tend to agree with you, I hope that Ryan will agree as well.

For as assertions the solution might be different in different scenarios, generally a typeof or an instanceof check costs basically nothing anyway.

From what I've seen most type assertion seem to be for objects which don't use prototypical inheritance (things like computation as Memo), so I don't think that we can use either of those.
I think that the only way is probably type guards which mean a separate function. I'm not sure if this means slightly bigger perf hit. For you it will probably still be negligible but Ryan seems to be very strict about perf so we'll have to see what he thinks.

Those scenarios could be solved via discriminated unions, for example maybe memos could have a memo: true, and then in the code you'd just write if ( computation.memo ) {}

Have mostly one file per exported function: having many many functions in a single file makes it harder to find the one you are looking for and to see all the exported functions at a glance.

I strongly agree that we should have smaller files, I even added that to the first post. I don't know if we should go to the other extreme though of one function per file. The way I like to work on personal projects is just mostly aim to maintain reasonable file sizes (I usually aim for around max 80 - 100 loc) while maintaining files as a logical module. I feel like too many tiny files could also make the code harder to work with when you always need to have lots of tabs open and toggle between them constantly and you can't see related code on the same screen. And there are also some very small functions like

export function enableScheduling(scheduler = requestCallback) {
  Scheduler = scheduler;
}

I feel like having a 3 line file would just lead to filesystem cluttering.

Those scenarios could be solved via discriminated unions, for example maybe memos could have a memo: true, and then in the code you'd just write if ( computation.memo ) {}

Could you provide a TS playground example? Cause as far as I can see this doesn't work.

I feel like having a 3 line file would just lead to filesystem cluttering.

Maybe, personally I'm fully invested in writing code that way (e.g. https://github.com/fabiospampinato/voby/blob/master/src/%24.ts). You can get a lot of files, but only when you are exporting a lot of things, everything is very clearly organized, which at least for me makes it much easier to reason about code, now I can open the file for "Show" or something and focus purely on writing that perfectly, if the code for Show is in a soup of other 100 functions it'd much harder for me to focus on it.

Could you provide a TS playground example? Cause as far as I can see this doesn't work.

Multiple ways to do that:

One letter variable names are not good

Groups of exports sounds like logical sets of files

However, I've also enjoyed having very large files.

Something that can really suck when breaking up functions with multiple indentation levels - is managing the typescript interfaces for the smaller chunk you are breaking out. But it can be worth it in long run.

It's funny, but adding TS and Doc headers increased line counts by like 125%. The original code in signal.ts is about 700 lines if memory serves. Still I tend to agree. I've been thinking about how to break up signal.ts without making a mess of circular dependencies. I've tried to do it twice in the past and just gave up after a few hours and was like later. You end up having to export a bunch more things and have them all reference each other. And then I remind myself I'm rewriting large parts of the reactive system coming up.

On TS types that's trickier. I don't find executable code there just for TS easier to reason about. Like functions that only purpose are to type guard. Mostly I still write and think in JS and keep it on the interface. Easy enough to move stuff around and keep interfaces fine. Adding functions for the sake of adding functions that may or may not having meaning as code changes seems bad. It isn't just about the extra code today but deciphering why it's there in the future and whether you still need it after refactor. To me if it isn't there for a runtime reason I'm not making the connection in the future. I look at the code see what it's doing. If it seems to be doing something for no reason that deserves investigation.

In general, I do agree these areas could be very much improved though. Especially single character variables. Tabs vs Spaces makes sense even if it would drive me nuts as the code length. I intentionally moved it to 100, since 80 pushes so many more lines. Again we have to acknowledge others. It's just you start in a place where you don't have any of this and the code is small and tight and viewable within a small space. And then you add more lines, and more comments, and more types, and longer names and bigger spaces and then you have no option but to break everything into more and more files. Ultimately everything becomes bigger looking and more spread out, and now it needs to be indexable, so you break things down more. And that's the inevitable conclusion. It makes it easier for contributors to approach piecewise. But when I look at those 50 files with 200LoCs each and rich documentation, I always lament the time when it could have been 5 files with 600 LoCs each. I can tell you which is easier to approach holistically for large-scale refactoring/rewrite.

It isn't just about the extra code today but deciphering why it's there in the future and whether you still need it after refactor.

IMO it's much easier to reason about something like that if you can rely on type checking compared to not being able to rely on it because everything has an accompanying type assertion.

At the end of the day though my opinion on this for what is worth is that the code should be written to serve whoever is writing it and whoever is executing it, if doing things one way would potentially benefit the 3 people writing the code but the rest of the people reading that code wouldn't like it then it probably should be done.

On TS types that's trickier. I don't find executable code there just for TS easier to reason about. Like functions that only purpose are to type guard. Mostly I still write and think in JS and keep it on the interface.

I personally find it more readable with type guards. The code is still there for runtime reason, at least when I worked on my refactor I just moved the if checks already existing into their own function which made it more declarative what those if checks actually do (with names like computationIsMemo) and also made the rest of the code shorter cause you don't need mutpile type assertions on the same branch.

And then you add more lines, and more comments, and more types, and longer names and bigger spaces and then you have no option but to break everything into more and more files.

Well you don't have to really ever see bigger spaces as tab length is configurable even on GH. But with tabs people who do need those bigger spaces can have them more easily. I did see that there is done debate about this since apparently modern text editor can automatically show spaces as if they were tabs.

At the end of the day though my opinion on this for what is worth is that the code should be written to serve whoever is writing it and whoever is executing it, if doing things one way would potentially benefit the 3 people writing the code but the rest of the people reading that code wouldn't like it then it probably should be done.

I disagree. This is not how accessibility works. Even if disabled people who actually benefit from those accessibility improvements were only a small part of the people who read the code and maybe none of the people who contribute (which is not true) we are still required to put disabled people's needs first especially for an open source project with 16k stars.

But when I look at those 50 files with 200LoCs each and rich documentation, I always lament the time when it could have been 5 files with 600 LoCs each. I can tell you which is easier to approach holistically for large-scale refactoring/rewrite.

I personally find every single accessibility enhancement I proposed to make the code also easier to work with for me or at least not make it harder. I wouldn't be surprised if with coffeescript without documentation or types and with things like single name variables the numbers you said are correct but without coffeescript I feel like it's probably a little bit exaggerated. Either way for me personally even the much longer code is easier to work with since every single thing that makes it longer also helps me. Not sure if everything that Fabio suggested will make it easier for me though like putting all types in their own files. I guess eventually a lot of it varies by person.

Use tabs instead of spaces

Oh no, that war again! I agree though :)

Reformat the whole codebase and limit line length to 79 or 78

Some folks also have very large zoom to read code.

[lower LOC per file is better]

I go back & forth. I hate files that are only 2-3 lines after imports & types. I also find having to untangle a web of imports & exports exhausting.

One letter variable names are not good [short var names are bad]

i, x, y are universally understood. j, z e are easy to figure out by the few who don't know the conventions. Is event, index, or innerLoopVar easier to scan & understand? Sometimes, but they also lengthen lines, thus eyes bounce right to left more. Maybe a middle-ground evt & idx is better?

Descriptive var names vs LOC & line length is a battle I'm waging now; my source:

const durationInSeconds = sequence.time.durationInSeconds ?? startInSecond - endInSecond
sequence.time.durationInSeconds = durationInSeconds
sequence..time.durrationFrame = durationInSeconds * FramesPerSecond

^ easier to understand, but more eye strain due to longer line lengths.

more terse refactor:

const durSec = seq.durSec ?? startSec - endSec
seq.durSec = durSec 
seq.durFrm = durSec * FPS

^ faster to scan & find operations, but strangers won't know what the variables 'mean' right away.

Middle ground refactor?

const durationSeconds = seq.timeDurSec ?? startSecond - endSecond
seq.timeDurSec = durationSeconds
seq.timeDurFrame = durationSeconds * FPS

I personally aim to keep var names less than 12 characters to help with scalability & LOC.

I much prefer full variable names from your first snippet.

I let prettier handle line length, I mostly don't worry about LOC until a file is REALLY long

Use tabs instead of spaces

Oh no, that war again! I agree though :)

I personally don't have much of an opinion, I just saw that people say online that tabs are more accessible and it sounds like it makes sense.

[lower LOC per file is better]

I go back & forth. I hate files that are only 2-3 lines after imports & types. I also find having to untangle a web of imports & exports exhausting.

I feel like it's best to find a balance. I personally prefer to keep most files somewhere between 30 and maybe 140 lines. I can see why it would make sense to sometimes have those very tiny files, mostly for things like export barrels, but I don't really understand huge files of this order (1650 lines, 1820 with 78 chars lines). I personally find dealing with those huge files very much more annoying then having to navigate through files or deal with imports, every time I'm in a function and I go to the definition of an inner function, I just lose the function I was in before, could be 1000 lines apart. Now I have to scroll 1000 lines again and search this specific function again in this huge file. I should probably start using the outline view in the explorer but even then

the outline is pretty huge and messy
if I want to be able to switch between the functions instantly I can't have them both open at the same time in different tabs without changing the layout, at least none that I know of, and even if I open them side by side it's kinda awkward to have the same file open in 2 different places. Also my screen is an ok size but not very big and side by side is not really comfortable on my screen.
I think that a lot of other devs don't use the outline either, probably mostly newer devs that didn't work with IDEs and languages like C# and Java.

One letter variable names are not good [short var names are bad]

i, x, y are universally understood. j, z e are easy to figure out by the few who don't know the conventions. Is event, index, or innerLoopVar easier to scan & understand? Sometimes, but they also lengthen lines, thus eyes bounce right to left more. Maybe a middle-ground evt & idx is better?

I personally don't see a reason to change generic one letter variables for looping indices, at least most of the time. As for e for event I'm not so sure, e is probably ok but I'd personally would prefer something like evt. Most of the ones I saw so far are things like s for signal, s for slot, s for source, s for suspense, s for suspense context (a lot of ss just to demonstrate how confusing it can get) stuff like that, also found a bunch of ls that I'm not always sure what they mean. Most of the time you can infer them from things like const c = createComputation(...) but I still feel like for most people who aren't used to read this code it just add mental overhead. Seeing this code that is kinda scary anyways for most devs, full with those one letter variables, makes it even more scary.

Descriptive var names vs LOC & line length is a battle I'm waging now; my source:
const durationInSeconds = sequence.time.durationInSeconds ?? startInSecond - endInSecond
sequence.time.durationInSeconds = durationInSeconds
sequence..time.durrationFrame = durationInSeconds * FramesPerSecond
^ easier to understand, but more eye strain due to longer line lengths.

more terse refactor:
const durSec = seq.durSec ?? startSec - endSec
seq.durSec = durSec 
seq.durFrm = durSec * FPS
^ faster to scan & find operations, but strangers won't know what the variables 'mean' right away.

Middle ground refactor?

I agree. I'm personally often ok with shorthands if they are very unambiguous and will most likely be understandable for almost anyone. All of those shorthands seem ok to me, maybe dur is slightly borderline but still ok. Things like comp for example I don't like cause it can be computation, computed, component, comparator and maybe even more relevant terms. I also saw a few things like obs for observers which could be probably understood by most people after looking at the code, but IMO adds mental overhead. I think that usually spelling out the full word has a relatively minimal cost in terms of code size.

const durationSeconds = seq.timeDurSec ?? startSecond - endSecond
seq.timeDurSec = durationSeconds
seq.timeDurFrame = durationSeconds * FPS
I personally aim to keep var names less than 12 characters to help with scalability & LOC.

In this case I think that it makes sense, but IMO very often it doesn't, in some cases 12 characters will make it very ambiguous and undescriptive, I saw that for example when working with Babel plugins and similar things, the complexity can get high pretty fast and it's pretty hard to follow anyway, and variables get ambiguous very fast cause you often deal with very specific stuff, and IMO the cost of long variables is often not comparable to the cost of ambiguity.
pathFromStitchesExtractToSrc
relativeImportPathWithForwardSlashes
openingTagClosingBracketLineNumber
After not looking at this code for a month this is hell to read anyway and shorter names will make it even harder. Maybe I could find shorter names that are still as clear but those are the names I came up with and coming up with shorter names will require additional mental effort for a reduction in a word or two for variables I don't reference a lot.
I think that in the Solid codebase we usually won't see a need for such long variables but IMO 12 characters is not a good hard limit or even a soft limit especially for a relatively popular open source framework. This in my opinion will definitely make the codebase less accessible, I think that probably mostly in the more complicated parts of Solid like suspense transitions and scheduling related stuff.

I would suggest to add prettier (+ eslint?) and husky with a pre-commit hook that runs before every commit. This should avoid having different code formats and you don't need to care about your format when writing. It will be formatted before the commit anyway. 🙂

If it's not annoying to work with different formatting locally this way it sounds good to me.

pathFromStitchesExtractToSrc
relativeImportPathWithForwardSlashes
openingTagClosingBracketLineNumber

Oh gosh, when I see variable names that long, my brain says blahNahNahBlahNah :)

Could be my concussion or ADHD (other accessibility issues to consider), but I can feel my brain hitting the breaks there. So too long variable names are no-go for me, though occasional long function names that are used in many files are OK.

YMMV: I do have a habit of 'scanning & not reading' from years of speed-reading & having to find issues in 1000 pages in 8 hour shift. Plus, I've programmed assembler (ONLY 8 'variables' for maths) for a few years, & programed on DOS (max 8 letter filenames) so having variable names even 12 char long seems like a luxury! 💸

I get that this can be harder for some people, but is it really harder for you to read then trying to guess what exactly the name means when you can barely follow what the code does anyway? I'm not sure if most frontend devs come across this situation very often, but when you are trying to build slightly complicated compiler stuff I feel like this is just unavoidable if you want people with average mental capacity to be able to actually follow the code without familiarity (or in my case even with familiarity). openingTagClosingBracketLineNumber is just an extremely specific thing and I feel like this is already pretty hard to follow and removing each part of it will probably make it more ambiguous and thus harder to follow. But like I said I don't think that we will need variable names like this for Solid (or that I would be able to convince you guys to use them anyway), my point is more that in some situations longer file names make sense and 12 will make the code way harder to read.

Maybe something that we can utilize here is JSDoc documentation for variables but for me personally it will still not really remove the need for those long names, I prefer the code to be understandable without having to hover over variables.

We should probably splinter/breakup this thread into a few different threads:
One would be about line length/prettier printWidth, for example. Others about file length and general organization, and others still about variable naming strategy

Yeah even using discussions would atleast allow responding to specific responses. I will move this later. But I agree once identified this is multiple things.

Navigation Menu

Repository navigation

Improve code accessibility #932

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Improve code accessibility #932

Description

Activity

fabiospampinato commented on Apr 10, 2022

fabiospampinato commented on Apr 10, 2022

orenelbaum commented on Apr 10, 2022

fabiospampinato commented on Apr 10, 2022

fabiospampinato commented on Apr 10, 2022

orenelbaum commented on Apr 10, 2022

fabiospampinato commented on Apr 10, 2022

orenelbaum commented on Apr 10, 2022

fabiospampinato commented on Apr 10, 2022

devinrhode2 commented on Apr 10, 2022

ryansolid commented on Apr 10, 2022

fabiospampinato commented on Apr 10, 2022

orenelbaum commented on Apr 10, 2022

tomByrer commented on Apr 17, 2022

devinrhode2 commented on Apr 17, 2022

orenelbaum commented on Apr 17, 2022

lockinmarv commented on Apr 17, 2022

orenelbaum commented on Apr 17, 2022

tomByrer commented on Apr 17, 2022

orenelbaum commented on Apr 17, 2022

devinrhode2 commented on Apr 18, 2022

ryansolid commented on Apr 18, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions