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

load images and fonts from executing dir #153

Merged
merged 2 commits into from
Dec 26, 2018

Conversation

akinsho
Copy link
Member

@akinsho akinsho commented Dec 23, 2018

Fixes #141 @bryphe hopefully this addresses the problem of loading fonts and images using esy x Bin (tested and does work) in the way you intended

@akinsho akinsho changed the title load images and font from executing dir load images and fonts from executing dir Dec 23, 2018
@jchavarri
Copy link
Member

This is awesome @Akin909 ! 🎉 Some related thoughts:

  • The esy.json file I think it's an artifact used by older versions of esy. In the most recent versions, it was replaced by an esy.lock folder
  • The addition of Sys.getcwd() probably will have some implications in projects targeting the browser through jsoo. js_of_ocaml doesn't support Sys that much, and from a quick test I did, it returns /static which doesn't make any sense in the folder structure of the test project I was using (there is no folder in that project called static). So maybe we have to create some abstraction over the different platform implementations like happened with fontkit or glsw. Maybe we could call it reason-system... or build it from rench (although, rench is more opinionated towards a Node.js-like API as far as I understand). I'd be happy to look into this as part of Fontkit.js support #116.
  • Probably out of scope 😄 but I'd love to play around ideas around handling compile-time known assets paths at build time. I'm still unsure how that would look like exactly, but my idea right now is to read these paths at compile time, maybe through a ppx, read the assets from the ppx binary, and convert the load expression into an assignment to a binding of the whole path binary data (maybe as a string? or as binary data?). Something like https://github.com/johnwhitington/ppx_blob.

@OhadRau
Copy link
Collaborator

OhadRau commented Dec 24, 2018

Awesome work so far! This is a great first step towards shippable revery software.

@jchavarri If you're interested in compile time asset loading, that's some thing I was gonna try to make actually. I've thought of a few ways of doing it and once I settle on one I'll go ahead and write a PPX for it:

  • Automatically guessing which files are statically accessible (if the path given to whatever File.load is a string literal && the file exists)
  • Having a [@static] annotation that can be used when loading a file to inform the compiler that it's a static asset
  • We can output each file as a linkable object file (e.g. https://www.linuxjournal.com/content/embedding-file-executable-aka-hello-world-version-5967) and generate extern declarations
  • We could also make an array or hashtable of strings (binary or otherwise) that we load, basically the same as what ppx_blob would do
  • For each of these, we have to let File.load know that these files are already loaded. We can do this by using a hashtable/other data structure of cached files and then inserting that as an optional parameter/overriding an empty data structure when we've loaded files

@akinsho
Copy link
Member Author

akinsho commented Dec 24, 2018

@jchavarri good point re. Sys not working in all environments 👍, you sound like you've given it way more thought than I did so for this PR I've just put a guard to only try the Sys call if the env is native, but an actual future solution that can do a similar thing per environment would be awesome.

Re. the esy.lock.json, it seems to have already been in the project, is it unnecessary now, seems to change whenever I run esy on a branch without necessarily changing dependencies.

The ppx idea sounds very cool (I'm new to reason and ocaml so would be beyond me at this point 😆 )

@akinsho akinsho force-pushed the feature/allow_executing_dir_imports branch from c5683da to 13662b6 Compare December 26, 2018 08:42
@bryphe
Copy link
Member

bryphe commented Dec 26, 2018

Just tried out this PR - I can now run esy x Bin.exe on Windows! 🎉 This makes it so much easier to run the demos (at least for me), and means #136 is unblocked now. Thanks @Akin909 for the change, and @jchavarri and @OhadRau for reviewing!

@jchavarri @OhadRau - that is a really cool idea for the compile-time asset loading! 😍 I don't want to lose the context of the discussion once this goes in, so I ported some of the thread over to #157

@akinsho akinsho merged commit f025d79 into revery-ui:master Dec 26, 2018
@akinsho akinsho deleted the feature/allow_executing_dir_imports branch December 26, 2018 18:36
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.

4 participants