MWS: Async rewrite #8888

Arlen22 · 2025-01-09T09:50:41Z

It was kind of fun. This took me about 12 hours, not counting interruptions.

It's not much of a rewrite, more just converting every database call (and everything upstream of every database call) to async/await.

If you haven't used VSCode, you're definitely going to want to explore the code using it to fully appreciate the little improvements that typing can bring.

I'm still working out a few bugs surrounding the somewhat confusing authenticatedUser property. But I'm too tired and I need to get some sleep and take a fresh look at it later.

github-actions · 2025-01-09T09:50:52Z

Confirmed: Arlen22 has already signed the Contributor License Agreement (see contributing.md)

netlify · 2025-01-09T09:52:04Z

✅ Deploy Preview for tiddlywiki-previews ready!

Name	Link
🔨 Latest commit	`0794b0a`
🔍 Latest deploy log	https://app.netlify.com/sites/tiddlywiki-previews/deploys/67805dfb8ec36200085aeeeb
😎 Deploy Preview	https://deploy-preview-8888--tiddlywiki-previews.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Arlen22 · 2025-01-09T16:51:02Z

Before I forget, this adds a NODE_DEV_PATH env variable which when set causes stack traces to print the actual file location instead of the tiddler name. This way you can just click on the line in the stack trace and it takes you directly to the file.

Arlen22 · 2025-01-09T16:52:51Z

It also allows startup and commander code to handle startups and commands that return promises while still maintaining backward compatibility.

pmario · 2025-01-09T20:18:00Z

Before I forget, this adds a NODE_DEV_PATH env variable which when set causes stack traces to print the actual file location instead of the tiddler name. This way you can just click on the line in the stack trace and it takes you directly to the file.

IMO this one is interesting in general. Can we have this one as a separated PR, or is this PR needed for that feature?

pmario · 2025-01-09T20:20:02Z

It also allows startup and commander code to handle startups and commands that return promises while still maintaining backward compatibility.

I think that's a "side effect" you wanted for a long time. -- "Sneaking in such functionality" does not work all too well, if this PR is rejected, the side effect is gone.

So is it possible to have this one as a standard PR, with only one goal

Arlen22 · 2025-01-09T22:23:33Z

It also allows startup and commander code to handle startups and commands that return promises while still maintaining backward compatibility.

I think that's a "side effect" you wanted for a long time. -- "Sneaking in such functionality" does not work all too well, if this PR is rejected, the side effect is gone.

I don't remember ever wanting that in those particular scenarios. I've only ever needed it in relation to the file system loading where no callback option was possible. And if I was trying to be sneaky about it I wouldn't put it by itself in the third comment of the PR. But now my lack of sleep is clearly showing!

So is it possible to have this one as a standard PR, with only one goal.

I wanted to make as few changes as possible to avoid cluttering up the git diffs and to make it easy to understand what would have to be changed to convert to async.

In order to do that it was easier to add the async keyword to startup and command functions and then add a bit of code behind the scenes to handle it rather than invoking callback hell while attempting to demonstrate how simple async/await can be.

It wouldn't be hard to convert the startups and commands to use the callback option that is already there. You basically just wrap the code in an async IIFE:

(async () =>  {
  // async function handled here
})().then(() => callback(), err => callback(err))

Arlen22 · 2025-01-09T22:48:09Z

Before I forget, this adds a NODE_DEV_PATH env variable which when set causes stack traces to print the actual file location instead of the tiddler name. This way you can just click on the line in the stack trace and it takes you directly to the file.

IMO this one is interesting in general. Can we have this one as a separated PR, or is this PR needed for that feature?

@pmario Sure. I've opened #8889 for that. Some further details are there.

pmario · 2025-01-10T00:45:13Z

plugins/tiddlywiki/multiwikiserver/modules/store/sql-tiddler-database.js

@@ -10,673 +10,678 @@ Validation is for the most part left to the caller

 \*/

-(function() {
+(function () {


There is a prettyprinting problem here. We do use TAB 4 as indentation in TW.
And there is no space between function()

pmario · 2025-01-10T00:47:54Z

plugins/tiddlywiki/multiwikiserver/modules/store/sql-tiddler-database.js

+  await this.engine.runStatements([`
+-- Users table
+CREATE TABLE IF NOT EXISTS users (
+  user_id INTEGER PRIMARY KEY AUTOINCREMENT,


Indentation is broken here. So it is not really readable anymore

pmario · 2025-01-10T00:48:27Z

plugins/tiddlywiki/multiwikiserver/modules/store/sql-tiddler-database.js

-	});
-	return updateBags.lastInsertRowid;
-};
+async createBag(bag_name, description, accesscontrol) {


no spaces between function parameters and TAB indentation

pmario · 2025-01-10T00:51:20Z

plugins/tiddlywiki/multiwikiserver/modules/store/sql-tiddler-store.js


-SqlTiddlerStore.prototype.removeEventListener = function(type,listener) {
+removeEventListener(type, listener) {


no spaces for params

Arlen22 · 2025-01-10T02:38:41Z

@pmario, is there a style guide anywhere that I can refer to? Is there some kind of prettier spec or eslint rule that's supposed to be applied?

I tried using eslint format, but the entire mws plugin is inconsistently formatted to begin with which just cluttered up the diff. II tried to do whatever caused the least amount of changes in the git diff so I tried to preserve the messed up formatting wherever possible.

That being said, I didn't do as well with sql-tiddler-database.js and the git diff there is particularly bad. I'll see if I can redo it without changing any formatting.

pmario · 2025-01-10T11:40:32Z

@pmario, is there a style guide anywhere that I can refer to? Is there some kind of prettier spec or eslint rule that's supposed to be applied?

There is an eslint-config.js in the main repo, that should be configured in a way that fits TW style-guide.
It seems in this PR you added some other stuff, that may interfere with that configuration.

pmario · 2025-01-10T11:42:58Z

I tried using eslint format, but the entire mws plugin is inconsistently formatted to begin with which just cluttered up the diff. II tried to do whatever caused the least amount of changes in the git diff so I tried to preserve the messed up formatting wherever possible.

Auto-formatting should be switched off. At the moment we still have the "function() wrapper" active. All the auto formatters mess up the indentation, because they do want to indent 1 level, that is not necessary.

There is a consensus, that this wrapper function will go away, because it is redundant in 99.9% of the cases. But that's not done yet.

arlen22 added 2 commits January 9, 2025 04:38

setup typing and linting

0cc52a3

handle promise based paths

48e56fa

Arlen22 mentioned this pull request Jan 9, 2025

MWS: Use Promises for sql-tiddler-database.js #8885

Open

arlen22 added 3 commits January 9, 2025 12:23

simple async changes

8c85ee5

store changes, first pass (ignore whitespace)

26d73c0

add await to tests (first pass)

1ec60ed

Arlen22 force-pushed the mws-async-rewrite branch from 14c7c36 to 1ec60ed Compare January 9, 2025 17:35

arlen22 added 5 commits January 9, 2025 18:05

only ignore store folders inside editions

873fb90

add eslint rule for ALL promises

f410d95

tweak NODE_DEV_PATH

548c7d0

more small catches and acl-middleware

316b89b

slightly complicated logical trees

0794b0a

pmario reviewed Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MWS: Async rewrite #8888

MWS: Async rewrite #8888

Arlen22 commented Jan 9, 2025

github-actions bot commented Jan 9, 2025

netlify bot commented Jan 9, 2025 •

edited

Loading

Arlen22 commented Jan 9, 2025

Arlen22 commented Jan 9, 2025

pmario commented Jan 9, 2025

pmario commented Jan 9, 2025

Arlen22 commented Jan 9, 2025 •

edited

Loading

Arlen22 commented Jan 9, 2025

pmario Jan 10, 2025

pmario Jan 10, 2025

pmario Jan 10, 2025

pmario Jan 10, 2025

Arlen22 commented Jan 10, 2025 •

edited

Loading

pmario commented Jan 10, 2025

pmario commented Jan 10, 2025


		SqlTiddlerStore.prototype.removeEventListener = function(type,listener) {
		removeEventListener(type, listener) {

MWS: Async rewrite #8888

Are you sure you want to change the base?

MWS: Async rewrite #8888

Conversation

Arlen22 commented Jan 9, 2025

github-actions bot commented Jan 9, 2025

netlify bot commented Jan 9, 2025 • edited Loading

✅ Deploy Preview for tiddlywiki-previews ready!

Arlen22 commented Jan 9, 2025

Arlen22 commented Jan 9, 2025

pmario commented Jan 9, 2025

pmario commented Jan 9, 2025

Arlen22 commented Jan 9, 2025 • edited Loading

Arlen22 commented Jan 9, 2025

pmario Jan 10, 2025

Choose a reason for hiding this comment

pmario Jan 10, 2025

Choose a reason for hiding this comment

pmario Jan 10, 2025

Choose a reason for hiding this comment

pmario Jan 10, 2025

Choose a reason for hiding this comment

Arlen22 commented Jan 10, 2025 • edited Loading

pmario commented Jan 10, 2025

pmario commented Jan 10, 2025

netlify bot commented Jan 9, 2025 •

edited

Loading

Arlen22 commented Jan 9, 2025 •

edited

Loading

Arlen22 commented Jan 10, 2025 •

edited

Loading