[meta] Client refactor or rewrite

[ix5]

Currently, the client is heavily DOM-dependent and written in ES5-compatible syntax.
Testing - although some great improvements with Jest and puppeteer have been made - is still hard to do.

We cannot currently make use of real Promise or await functionality and other ES6 goodies. The justification for sticking with older source code syntax has been laid out in #834, but this is not official policy but rather the opinion of 1 maintainer, subject to change.

Things to think about:

• Compatibility with older browsers
• Increasing complexity of developer setup (installing too many packages, handling too many configs)
• Notoriously unreliable and insecure JS/npm ecosystem
• Bundle size
• Performance with many comments, memory leaks when leaving page open (setTimeout)
• Single-Page-Applications (SPA) and carrying state

New or improved features

• Editing buttons to insert Markdown elements like ** around the cursor (like GitHub, remark42, ...) (idea: cursor)
• Editing help (inline, as a little popover or modal that slides out)
• Editing name/website (and possibly email), tracked in Make name/URL/e-mail editable #111
• Re-using more front-end functionality in the admin interface
• Cleaning up CSS (tracked in [client] Clean up and standardize client CSS #859)
• Auto-sizing <textarea>
• Better feedback on errors (e.g. Isso doesn't show or display errors #673)
• Wishlist: Javascript-free client, Python API #331

See also

• An experimental rewrite of the client, with some new ideas for extensibility
• Ground-up rewrite using React and TypeScript https://github.com/BBaoVanC/isso-frontend
• Discussion in js, templates: Replace contenteditable div with textarea #887 (comment)

[BBaoVanC]

Continuing from the discussion in my PR

TS sure looks like a nice addition, but we might also be limiting ourselves in the contributors we get - they'd have to know TS as well. This kind of feels a bit like the "sprinkle some async" fad in python to me. Maybe start with better test coverage and work from there?

Better test coverage is a great start. TypeScript is very similar to JavaScript; it looks to me that it would be easy enough to understand quickly, especially if the contributor is familiar with other static/strongly typed languages.

I tried to look into TypeScript's popularity, and according to GitHub, TypeScript is 4th, while JavaScript is 1st. Stack Overflow's developer survey lists TypeScript as the 3rd most loved language, behind Rust and Clojure, and JavaScript is 15th.

I won't look into that too much because it's a very opinionated point. But it is cool to see.

I'm already kind of annoyed at all the Javascript packages constantly jumping sub-revisions (e.g. Jest and puppeteer all jumped several since I added them, and they need to be kept in sync with system chromium version if you don't use the .local-chromium version). You're never quite sure if something just broke because of a version change of a dep when re-installing or you broke something yourself. webpack is already quite a moving target and has not been pleasant to set up. I'm not really looking forward to spending maintainer time accepting PRs bumping revisions from bots every few days (new tsc release here, new babel release there, ...). It's not too much to do, but death by a thousand cuts.

I feel that with the proper tests, then using dependabot and pinning the dependencies to an exact version would be good. Ideally, the CI could run the tests on dependabot pull requests, which would catch any behavior differences the new package adds. That combined with the screenshot tests would hopefully mean that you don't have to do any manual testing on the new dependency.

There should only be pull requests for the top-level dependencies (in this case, webpack, webpack-cli, jest, and jest-environment-jsdom, plus one or two more if using TS) if using dependabot. There wouldn't be pull requests for dependencies of dependencies (and so on).

---

If we're going to want to start using ES6 features, TypeScript would allow us to use them, but transpile an ES5 JS output. We could also use classes, types, and interfaces to represent data in a more object oriented and organized way.

I don't know if I'm really explaining this well, maybe I'll have to play around with it first before I can.

See also:

• TypeScript for JavaScript Programmers
• Classes - TypeScript

[BBaoVanC]

While looking at the experimental client code, I think I sort of see how prototype is used to create classes. To me, this is really messy and hard to understand. I tried to make a quick example of what I think the example you put in the README.md.

JS

var App = function(conf) {
  this.conf = conf;
}
App.prototype.initWidget = function() {
  var self = this; // Preserve App object instance context
  renderSomething(self.conf);
  // [...]
}

// later, in other file:
var app = new App('conf');
app.initWidget()

TS

class App {
  constructor(conf) {
    this.conf = conf;
  }
  initWidget() {
    var self = this; // Preserve App object instance context
    renderSomething(self.conf);
    // [...]
  }
}

// later, in other file:
var app = new app.App('conf');
app.initWidget()

To me the TypeScript example is a lot cleaner and more familiar. I actually feel like TypeScript would be easier for people to learn if they don't know JavaScript at all. Another advantage is that editors (such as Visual Studio Code) can give much better suggestions because of the static nature of TS.

I asked a few friends who do web development and they told me that your JS example is the normal way to do it in JS, but also that TS is worth it.

I think I'll still make my own experimental version, but I'll try and use modern frameworks and all so I can learn them, and so it's not just a second pointless rewrite.

---

Also I found out that adding TypeScript adds only two packages total to the entire ~500 package tree¹. And it takes no extra work on the developer's side since Webpack can be easily configured to just transpile the TypeScript before it does its bundling. I tried this on the experimental client, see BBaoVanC/isso-client-experimental@749e5862f37bf28ef61a34fe33e3ff46ad4d485f²

Footnotes

1. It only requires adding two NPM packages: typescript and ts-loader. typescript has no dependencies, and the four dependencies of ts-loader are already included in our tree due to other packages that depend on them. ↩

2. This commit does not build because I didn't rename the .js files to .ts in it. I wanted this commit to be clean and just show the config changes. ↩

[ix5]

That TS only pulls in one mono-package (plus a few for ts-loader) looks very appealing to me. You can progressively start using TS/modern features and use TS to compile back to ES5.
If it's that easy and we can skip adding all the babel-related bloat, I'm all for using TS.

I added you as a collaborator to the repo, feel free to push.

[BBaoVanC]

I've been taking a look and I feel like the way the experimental rewrite is structured is more complicated or harder to understand than the original source here. And I don't know if rewriting it in TS is as much worth it as I thought.

Currently I'm trying to figure out if I can make my own version in React & TS to see if it can bring any better structure (hopefully without making it too complicated either), but it'll be a bit before I can know for sure. Hopefully using React to handle internal structure could make it easier to develop on top of. I'll show it soon if I can get a good working client in it.

Maybe React.js and it's component-based structure makes it more organized to add extensions? This is something I'll look into as well.

I'm also worried about whether rewriting anything is worth it, compared to just gradually improving the existing code.

[ix5]

I share your hesitation with rewriting things from scratch. You'll notice that the experimental client still shares most of its code with the stock one, so a "rewrite" is probably not the correct term to use.
I had to make a few structural changes to accommodate the following issues (which I still don't consider fixed):

• A lot of places rely on the config object/instance. The config, however, needs to be fetched from the server first (or rather, the overrides).
• The api needs to know its own endpoint, so it has to be instantiated or behave object-y, if we don't want to let it scrape the endpoint from script tags on every API invocation
• The postbox and comment renderers can insert instances of each other, i.e. the a "comment" needs to be able to insert a postbox for replying and a postbox needs to insert comments once they're submitted.
• There are a lot of periodic tasks (updating ago datetime to human representation) to be done, all very similar. They should be handled in a "clock tick" fashion and not each run on their own (else we'd have a setTimeout timer for each) They should vanish as soon as the underlying DOM object is gone.
• Everything that translates things needs an i18n instance, which in turn needs to have a config instance for language and pluralforms selection
• All template rendering needs a config and readily-configured i18n instance
• init() and fetchComments() both need to wait for the config to be fetched from the server. And they need a sort of locking/waiting mechanism to block until that is done, or defer those actions that rely on config until it is fetched. Also, init needs to be able to tell fetchComments to start working, so sharing some sort of mutex is needed.

So a lot of hen-and-egg problems, where we'd need some kind of central arbiter that listeners can register with, that is responsible for keeping state.

If adding a framework such as React helps with that or enforces some common best practices, I'm all for it. But I'd rather avoid adding too many dependencies and forcing us into some patterns before we've each deemed that change beneficial and necessary.

I'd propose we each do some more research, experiment a bit and then revisit this topic.

[BBaoVanC]

Interesting news, IE is now out of support as of today. And it was really the only reason we weren't using more modern features like CSS variables and ES6 right?

What do you think about updating some of the CSS and JS in the near future?

[ix5]

M$ unilaterally declaring something out-of-date doesn't magically make people stop using it.
The decision to maybe wait a bit on "modern" features such as CSS vars etc was based on percentages from caniuse, so not only IE versions, but all browsers that end users currently (still) use.

That being said, if we get ES5 compat "for free" when using TypeScript, I'm all for that.

CSS vars: Caniuse currently states 94% browser support.
I don't think we necessarily need a "technical" solution to that issue (mismatching/disarrayed use of "random" colors).
What if we listed all used (main) colors on top of the file as comments and only expressed opacity-reduced usage as rgba()? (rrggbbaa isn't well-supported, clean that up as well)