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

Allow file reading on mobile #9

Closed
fenjalien opened this issue Jul 23, 2023 · 18 comments
Closed

Allow file reading on mobile #9

fenjalien opened this issue Jul 23, 2023 · 18 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@fenjalien
Copy link
Owner

fenjalien commented Jul 23, 2023

All file reading in Obsidian has to be done using async/await due to the mobile version using https://capacitorjs.com/ which only supports reading of the file system asynchronously. Unfortunately compiling Typst only works synchronously, you can start it in an asynchronous environment but as soon as it needs to read a file you are stuck in synchronous (you can't get to async from sync).

My original work around of this was to cache all of the .typ files in a Map before hand so it can be accessed in sync. This doesn't necessarily scale well with large vaults and numerous packages.

So now the compiler now a web worker, which effectively means its own thread. The worker can safely make a request to the main thread to send it a file and then block while it waits for the main thread to read it asynchrnously. Because the worker blocks while waiting for a file the normal onmessage callbacks cannot be used, so instead the file is put into a SharedArrayBuffer and Atomics are used to wait and notify the worker.
If this method was to work in a normal browser certain CORS would have to be set in order to enable SharedArrayBuffers which I'm not sure is possible in Obsidian. However for some reason they are available anyway on desktop, but not on mobile. At the point I realised that it would not work on mobile it was a bit too late to scrap it.

So to fix this either:

  • Find a way to pass data between threads without the use of a SharedArrayBuffer
  • Find out how to enable SharedArrayBuffers on capacitor js
@fenjalien fenjalien added enhancement New feature or request help wanted Extra attention is needed labels Jul 23, 2023
@Myriad-Dreamin
Copy link

Myriad-Dreamin commented Aug 1, 2023

I'm not sure whether you have known the the approach that I will pose here: You can bootstrap another service worker which handles synchronized xmlHttpRequest from Wasm module. In service worker, you can process async calls, then response to the Wasm module1. One of advantage is that the approach does not rely on Atomics with SharedArrayBuffer at all. I believe you knew it:

  • a web worker can be safely blocked by other threads.
  • SharedArrayBuffer has limited support in current browsers because of the spectre attack.

But by my old experiment, there are possible logical bugs and performance issues in browsers' synchronized xmlHttpRequest. They won't fix it, since synchronized xmlHttpRequest is deprecated. It still works though and is used by current many polular web services2.

Footnotes

  1. https://medium.com/@maulanamaleek/intercept-http-request-using-serviceworker-b6ef23f97d1f

  2. even by typst's webapp.

@fenjalien
Copy link
Owner Author

I like the idea and does mean I could move the compiler back into the main thread as its a lot of work. The only thing I'm not sure about is whether or not the service worker could then read the files... I'll have try it out.

@kwinso
Copy link

kwinso commented Sep 4, 2023

Hello. How is the progress with this issue? I'm really looking for this feature, I really don't want to return back to LaTeX just to be able to read my formulas on mobile. By the way, is there any way I can help with speeding up progress for this?

@fenjalien
Copy link
Owner Author

This actually slipped my mind for the last update, having another look its definitely possible and honestly makes the code much simpler. I'm not sure when I'll get back to this as I do have other priorities but hopefully not long after Typst 0.8 releases at the latest.

@kwinso
Copy link

kwinso commented Sep 9, 2023

@fenjalien I'm ready to contribute and make this feature come out faster. Can you please share what exactly you wanted to change, and I'll do a PR with this change. As I can tell by looking at the source code, the main deal is in the requestData function? Thank you in advance.

@fenjalien
Copy link
Owner Author

@kwinso Thanks for volunteering! I can't really say exactly what needs to change, but the main point would be to make it work without the use of sharedarraybuffers. Hopefully to the point of no buffers at all and instead use strings. Another thing is to make sure the fs module isn't used as it doesn't exist on mobile, you'll still need it to read packages on desktop though.

btw I don't think I'll have the time to merge and do a release for just a single PR. But you should be able to load your fork onto your device until then...

@kwinso
Copy link

kwinso commented Sep 10, 2023

I found another issue. I've completely removed file reading from the plugin, so there's no shared array buffer logic at all. But it still crashes (initially I thought that crashes are caused by file reading). I think it may be something related to WASM loading.

@kwinso
Copy link

kwinso commented Sep 12, 2023

Did it crash before on your device @fenjalien? I think it's an issue with wasm module size (25 mb main js file for my build), so react native environment (I believe this is what's used for obsidian mobile) isn't able to handle this size

@fenjalien
Copy link
Owner Author

hey @kwinso I'm around to do this now, how far through are you?

@kwinso
Copy link

kwinso commented Sep 17, 2023

@fenjalien I actually didn't do much progress in this, had a lot of things at my uni. But the main issue for me is it's just crashing on mobile. Obsidian starts loading plugins and then crashes, if the obsidian-typst plugin is present. I would love to work at this plugin, but I just can't test any new functionality until the issue with crashing is resolved. Maybe it's something to do with the actual size of the plugin? main.js file is around 25MB. I've looked into typst.ts thing, and it's around 14MB.

My thoughts is that react native (or whatever obsidian mobile is built with) is not able to load such big file.

Also, there's no logs or something in the mobile version (or I don't know how to enable it), so I can't properly debug what's going on

@kwinso
Copy link

kwinso commented Sep 17, 2023

But regarding the issue with file reading, I think it might be worth to update the Rust code to be asynchronous, so plugin can call Obsidian async functions directly from Rust. That way there will be no need to implement a requestData function at all

@fenjalien
Copy link
Owner Author

fenjalien commented Sep 17, 2023

I would love to work at this plugin, but I just can't test any new functionality until the issue with crashing is resolved. Maybe it's something to do with the actual size of the plugin? main.js file is around 25MB. I've looked into typst.ts thing, and it's around 14MB.

My thoughts is that react native (or whatever obsidian mobile is built with) is not able to load such big file.

No its because I selected the wrong distribution of svgo which required node which doesn't exist on mobile. I've fixed that in a recent commit so it should work now. I've had a look into reducing the bundle size but apart from splitting the bundle into separate files I've not had much success. It uses capacitor js btw.

But regarding the issue with file reading, I think it might be worth to update the Rust code to be asynchronous, so plugin can call Obsidian async functions directly from Rust. That way there will be no need to implement a requestData function at all
Sadly it just doesn't work like that :/ Once you call Typst's compile function its within the synchronous domain and so the callback to request data must be synchronous.

@kwinso could you push to your fork so I can see what you've done?

@kwinso
Copy link

kwinso commented Sep 17, 2023

@fenjalien I have not really tried anything, it was just my thought when I was poking with the codebase. But I'm going to check the new build with updated SVGO, and if it's working on mobile, I'll try something with async code.

@kwinso
Copy link

kwinso commented Sep 17, 2023

@fenjalien I tried latest commit, it's not crashing anymore. But nothing is rendered. Is it caused by the issue with mobile file reading, right?

@fenjalien
Copy link
Owner Author

@kwinso I'm not sure whats going on without know exactly what you've changed (i.e. without reading your code) but currently all file reading on mobile is disabled so that shouldn't be a factor.

@kwinso
Copy link

kwinso commented Sep 17, 2023

So, as my initial goal was to allow math reading on my mobile phone, I thought that whole Typst typesetting system may be overkill for this, so I created this plugin. It allows to use Typst math syntax. Also, I've included link to your project for anything other than just math. So, I think, I'll proceed to maintain this project instead of trying to help you, as I don't have that much time to maintain more complex plugin, just like yours.

I wish you much luck with all the issues! If I somehow have more time, I'll would love to contribute here.

@kwinso
Copy link

kwinso commented Sep 19, 2023

Sadly it just doesn't work like that :/

Didn't see this at all, it was formatted as a quote, I thought everything is my words. Noticed it just now.

So, let's come back to your initial thought with strings instead of buffers. It is doable, the only thing is that we need to somehow wait until pluging returns the file. What about synchronized promise, or directly the deasync package? We can wrap file reading with this. At least attempt to do this :)

@fenjalien
Copy link
Owner Author

FINALLY FIXED with 0.8.0!!!!

Turned out on mobile theres a http endpoint you can query which basically reads the file system. I also found out that for some reason you can't even create service workers on the desktop or mobile anyway so that would have never worked :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants