ankitects / anki

Anki's shared backend and web components, and the Qt frontend
https://apps.ankiweb.net
Other
18.88k stars 2.14k forks source link

Investigate switch from node to deno #2439

Closed dae closed 7 months ago

dae commented 1 year ago

Some issues we're likely to hit that will need investigating before it becomes practical:

Related: #2430

andrewpeters9 commented 1 year ago

Happy to take this on if you'd like me to as I have decent availability - feel free to assign it to me if you're comfortable with me taking this on. Will also be able to help with the actual conversion.

Just some other trivial things I think could be added to the checklist:

dae commented 1 year ago

If you'd like to look into this, that would be much appreciated. A couple of points:

andrewpeters9 commented 1 year ago

Took a bit of look today @dae .

dae commented 1 year ago

Thanks Andrew.

Currently package.json is rather lax compared to the lock file; after 2.1.61 is released (hopefully not long) I'll do a dependency bump and update package.json at the same time so they're closer in sync.

I don't mind a temporary period where we have both node and deno being used, as long as there's a clear path to full migration. What would not be ideal is if the deno migration stalls, and we end up stuck requiring both of them.

andrewpeters9 commented 1 year ago

@dae Gotcha. I'll dig deeper into these testing Jest and protobuf issues over the weekend - although I may need a hand with ascerning if protobuf will still work, as I'm new to Rust + protobuf.

I'll also start to put together a list of tasks required, and how they could be potentially batched up.

dae commented 1 year ago

A smoke test for the protobuf code is opening the graphs screen - the graphs will fail to render if protobufjs is not working. If you run into an issue with it and need help, please let me know.

andrewpeters9 commented 1 year ago

A smoke test for the protobuf code is opening the graphs screen - the graphs will fail to render if protobufjs is not working. If you run into an issue with it and need help, please let me know.

Thanks for the tip. I'm still working on this by the way, I've just been smashed with other work recently. Will be progressing this weekend.

dae commented 1 year ago

No rush - this is not a blocking task. Appreciate your contributions.

dae commented 1 year ago

Still no rush on this, but just wanted to let you know I've gotten around to porting to protobuf-es, which should be more deno friendly, and reduces the number of dependencies we need. This also removed the need for patch-package.

https://github.com/ankitects/anki/pull/2547

andrewpeters9 commented 1 year ago

Hey @dae , I'm still wanting to do this, have just been smashed at work recently. Jumped back on this over the weekend, here's what I've found/did:

Some recent developments to Deno:

dae commented 1 year ago

Thanks for all your work on this Andrew.

Jest pulls in quite a lot of transitive dependencies IIRC, so if it's practical to replace with Deno's built-in runner, that would be a nice win. Some of our Jest tests use it with the jsdom environment, so we can run tests against browser APIs (eg ts/html-filter relies on document.getElement() & .nodeType), so we'd need some way to still support those APIs (maybe deno-dom + some reworking of the tests?).

Re lock files, in larger Anki updates, I try to update all deps to their latest. In smaller updates, I want the versions to be locked, and the ability to update single deps (eg pull in a security update to a package without updating everything else). I presume such a workflow can be done using the traditional Deno approach of listing out versions in a dependencies.ts file, which the rest of the codebase imports? A move to deno would come as part of a larger update, so I'm not too concerned about matching the existing yarn lockfile. I briefly Googled the deno lock documentation, but it seemed pretty barebones - I'm guessing it doesn't currently provide something akin to 'yarn upgrade' or 'yarn upgrade some_package'?

I gave the math-jax task a try using your PR, and was initially surprised at how slow deno was to pull in all the packages, but then I realised that I'm just used to yarn runs where the packages have already been cached.

Re @tslib imports in VS Code, while we could reference the non-generated files via relative paths instead, all our protobuf code and translations are also referenced that way, so we'd definitely need to fix this before a switch is practical.

I'm open to starting with deno as just a Jest replacement, once we've at least prototyped using it for our other components and/or can easily roll it back. What I'd like to avoid is a situation where we've partially switched to deno and then discovered showstopper issues that require us to keep using yarn, and we're unable to roll back deno either, leaving us with a build system that is twice as slow :-)

andrewpeters9 commented 1 year ago

Jest pulls in quite a lot of transitive dependencies IIRC, so if it's practical to replace with Deno's built-in runner, that would be a nice win.

Sounds like a good MR to start with. I'll start work on this over the weekend.

I presume such a workflow can be done using the traditional Deno approach of listing out versions in a dependencies.ts file, which the rest of the codebase imports?

Correct, but we can also just keep the package.json to manage versions, now that deno has support for this: https://deno.com/manual@v1.34.3/node/package_json

NOTE: deno does still recommend using a deno.json file for import mapping, so maybe migration from package.jsondeno.json becomes a house-keeping task once the conversion is fully complete. I'll make a GH issue for this so that I don't forget.

I briefly Googled the deno lock documentation, but it seemed pretty barebones - I'm guessing it doesn't currently provide something akin to 'yarn upgrade' or 'yarn upgrade some_package'?

For upgrading all packages in one command, I'm unsure if this exists, but individual packages can be updated while still being careful not to disrupt the lockfile. So I don't think there's a problem here.

I gave the math-jax task a try using your PR, and was initially surprised at how slow deno was to pull in all the packages, but then I realised that I'm just used to yarn runs where the packages have already been cached.

Necessary packages are cached upon the first call to deno, so subsequent builds should be a lot quicker. Feel free to re-run that command to check the speed - as I don't want your workflow to be impacted by a command that I imagine you're running plenty of times a day.

Re @tslib imports in VS Code, while we could reference the non-generated files via relative paths instead, all our protobuf code and translations are also referenced that way, so we'd definitely need to fix this before a switch is practical.

Noted.

I'm open to starting with deno as just a Jest replacement, once we've at least prototyped using it for our other components and/or can easily roll it back.

Sounds good to me!

dae commented 1 year ago

For upgrading all packages in one command, I'm unsure if this exists, but individual packages can be updated while still being careful not to disrupt the lockfile. So I don't think there's a problem here.

Having to manually update all packages would be rather cumbersome, and I'm a bit surprised they don't provide a first-party solution for this, but it looks like https://github.com/hayd/deno-udd will accomplish it.

so subsequent builds should be a lot quicker.

Yep, they are :-)

dae commented 7 months ago

Closing, as it was determined that it's not practical for now.