Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MWS: Async rewrite #8888

Open
wants to merge 26 commits into
base: multi-wiki-support
Choose a base branch
from

Conversation

Arlen22
Copy link
Contributor

@Arlen22 Arlen22 commented Jan 9, 2025

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.

Copy link

github-actions bot commented Jan 9, 2025

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

Copy link

netlify bot commented Jan 9, 2025

Deploy Preview for tiddlywiki-previews failed.

Name Link
🔨 Latest commit 4026a40
🔍 Latest deploy log https://app.netlify.com/sites/tiddlywiki-previews/deploys/6785f8b40a318d0008b72b3f

@Arlen22
Copy link
Contributor Author

Arlen22 commented Jan 9, 2025

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
Copy link
Contributor Author

Arlen22 commented Jan 9, 2025

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

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

pmario commented Jan 9, 2025

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
Copy link
Member

pmario commented Jan 9, 2025

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
Copy link
Contributor Author

Arlen22 commented Jan 9, 2025

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
Copy link
Contributor Author

Arlen22 commented Jan 9, 2025

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.

@Arlen22
Copy link
Contributor Author

Arlen22 commented Jan 10, 2025

@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
Copy link
Member

pmario commented Jan 10, 2025

@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
Copy link
Member

pmario commented Jan 10, 2025

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
Copy link
Contributor Author

Arlen22 commented Jan 10, 2025

@pmario I redid the store folder using an eslint rule, which was much cleaner and preserved the original formatting pretty exactly, whatever that was, which makes the diff cleaner. Somehow it still managed to switch single quotes to double quotes. No idea. It feels like git is running something after I commit but I have no idea.

Sorry, I'm just trying to propose a change to how the MWS plugin is written: one that I feel is very important, and I'm trying hard to communicate exactly what the change would entail as cleanly as I possibly can. I really don't want to fix other people's formatting when I'm not a primary contributor here.

I did notice that eslint has an indent rule with an IIFE option. You'll have to play around with it though because it has some funny defaults.

}

async saveTiddlersFromPath(tiddler_files_path, bag_name) {
const attachmentsEnabled = this.adminWiki.getTiddlerText("$:/config/MultiWikiServer/EnableAttachments", "yes") === "yes";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation problem :/

@Arlen22
Copy link
Contributor Author

Arlen22 commented Jan 10, 2025

@pmario, Per #8885 (comment), I added a simple worker thread to handle the SQLite calls, but then I realized that the current sync setup is the only thing keeping transactions in order, so it's broken until I figure that out.

@pmario
Copy link
Member

pmario commented Jan 13, 2025

@Arlen22 ... Can this be tested? If yes how? -- Did you try Deno2 or Bun to run .ts directly, without the whole transpiler overhead?

@Arlen22
Copy link
Contributor Author

Arlen22 commented Jan 13, 2025

Not yet. I'm still converting the code.

@Arlen22
Copy link
Contributor Author

Arlen22 commented Jan 14, 2025

@pmario, in order to run it you need to run the following code:

npm install # there are some new deps
npm run prisma:generate # generate the prisma client code
npm run mws:create # delete and create the sqlite db
npm run arlen # this rebuilds the typescript code and starts the server

@Arlen22
Copy link
Contributor Author

Arlen22 commented Jan 14, 2025

I'm still working on transactions. But for a proof of concept, I'd say I've proved about enough. It works... but there is so much type weirdness going on throughout the codebase that I'm not sure exactly what's working and what's broken without testing every last code path.

Normally when I'm working with Typescript everything is straightforward. All the types are right in the tooltip, and I get errors whenever I mess something up. Obviously that's not happening here, although there are some things that Typescript is catching even in the Javascript modules.

I recognize that MWS is an amalgamation of the existing server module with a bunch of extra things bolted onto it. I'm not sure if that's good, but I guess this is in the early enough stage that it's not a big deal. I'm sure someone is going to be doing some more passes on MWS and straighten everything out.

That being said, I'm so glad to see this plugin finally making it into the plugin folder. We've needed this for a long time. I'm looking forward to it being finished and being able to use it for all kinds of stuff. I don't know what it will look like exactly, but I've always felt like there's so much untapped potential in the multiwiki concept.

Anyway, I think that's all I'll be doing with this particular proof of concept for the moment. The existing codebase isn't clean enough for me to be able to convert it myself. I really think in Typescript. I would seriously love to use Typescript in TiddlyWiki, but last I heard that's not happening.

I'll leave it open for now and I might come back to it over the next few weeks, I'm not sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants