lv_binding_js icon indicating copy to clipboard operation
lv_binding_js copied to clipboard

Use txiki as runtime

Open jspngh opened this issue 1 year ago • 4 comments

First of all, thanks for all the work in this project. I've been using it for some time now. I did notice some opportunities for improvement, which have resulted in quite a big change, but I do believe they would be beneficial for the future of the project.

Since the runtime included here is derived from txiki.js, I wanted to see if it's not possible to directly depend on it instead of having to copy-paste part of it in here, which gets out-of-date quickly. By having a direct dependency, keeping up-to-date with it should be easier, and you get access to all features.

I also took the liberty of making other improvements, like removing the *.js files and having them built from the *.jsx files with node build.js.

Some notable differences between this and the current implementation:

  • you have to use lvgljs run $file to execute a file
  • it will look for files (e.g. assets) in the current directory (when using a relative path) instead of the directory of the .js file being executed
  • the require syntax of node is currently not supported by txiki

Since the diff of this PR is quite big, I suggest reviewing the different commits, which I have tried to structure logically.

jspngh avatar Jun 14 '24 16:06 jspngh

This is 🔥! I will review and test locally in the next few days! Having better support for Web APIs is great, as well as is having a proper event loop, so thank you for sending this!

derekstavis avatar Jun 26 '24 23:06 derekstavis

I started reviewing this. It's definitely a lot but it all makes sense. A few initial thoughts:

  1. I'd suggest we now add a make setup target that deals with: a. All the submodule madness. Since now modules are recursive it's a bit of a pain. b. Installs node dependencies. I forgot and node build.js failed.
  2. Having a target that builds + runs the simulator would be a nice to have. a. This could deal with the working directory situation. b. It's annoying rn: make simulator; cd demo/widgets; ../../build/lvgljs run widgets.js
  3. I think it would be good to update the README file with some instructions.

Beyond that, this is working great on my setup!

If you don't happen to get time to work on those it's fine. I should be able to merge myself and add them.

I have an ongoing branch migrating to lvgl v9 (so that I can support custom fonts) but I will get this merged first.

derekstavis avatar Aug 01 '24 23:08 derekstavis

This is overall looking great. I'm keen to merge. I'll wait for your input and if I don't hear back I'll integrate it into main. This is a significant improvement in cleanliness of the project, and I think it's super welcome.

Thanks for the review. I'll have a quick look if I find a solution for your globalThis[Symbol.for(...)] comments. If I don't come up with something right now, that can be a separate PR.

I'll also update the documentation and create a:

  • make setup target that initialized the submodules and runs npm install
  • make demo target that builds and runs the simulator with the widgets demo I'll update the src="..." paths to start from the repository root, so you don't have to cd to the demo/widgets directory.

jspngh avatar Aug 02 '24 07:08 jspngh

I'll have a quick look if I find a solution for your globalThis[Symbol.for(...)] comments. If I don't come up with something right now, that can be a separate PR.

Yes we can leave it up to a separate PR if you find it better.

I'll update the src="..." paths to start from the repository root, so you don't have to cd to the demo/widgets directory.

I think that the entrypoint file (the one passed to lvgljs run <file.js>) directory is a sensible default for looking for assets, but I'm not opposed to the CWD being the standard, as it's how most applications work. The only annoyance is during development really.

derekstavis avatar Aug 03 '24 03:08 derekstavis

Documentation and Makefile are updated, together with some additional fixes.

I'll have a quick look if I find a solution for your globalThis[Symbol.for(...)] comments. If I don't come up with something right now, that can be a separate PR.

Yes we can leave it up to a separate PR if you find it better.

The fix for path import was easy, but for the lvgl bridge we'll need a different solution. I propose to tackle it in another PR.

I'll update the src="..." paths to start from the repository root, so you don't have to cd to the demo/widgets directory.

I think that the entrypoint file (the one passed to lvgljs run <file.js>) directory is a sensible default for looking for assets, but I'm not opposed to the CWD being the standard, as it's how most applications work. The only annoyance is during development really.

Yes, having the path be relative to the JS file could be nice. But I would also postpone it, since I won't have the time to look at it in the coming days.

jspngh avatar Aug 04 '24 06:08 jspngh

Great work here! Thank you so much for the migration!

derekstavis avatar Aug 05 '24 01:08 derekstavis