isso-comments / isso

a Disqus alternative
https://isso-comments.de
MIT License
5k stars 442 forks source link

[meta] Client refactor or rewrite #894

Open ix5 opened 2 years ago

ix5 commented 2 years ago

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 https://github.com/posativ/isso/pull/834, but this is not official policy but rather the opinion of 1 maintainer, subject to change.

Things to think about:

New or improved features

See also

BBaoVanC commented 2 years ago

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:

BBaoVanC commented 2 years ago

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 ```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 ```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[^two-packages]. 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[^ts-commit]

[^two-packages]: 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.

[^ts-commit]: 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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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):

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 commented 2 years ago

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 commented 2 years ago

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)