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

Remove buildsInSource #136

Merged
merged 4 commits into from
Dec 27, 2018
Merged

Conversation

jchavarri
Copy link
Member

Same as revery-ui/reason-reactify#32 but for revery.

Not sure about the paths in README as I don't have a Windows machine, but happy to fix them if they're wrong. 🙂

@bryphe
Copy link
Member

bryphe commented Dec 22, 2018

Thanks @jchavarri for thinking about this! Definitely makes sense to change - want a good language service story for working with revery.

Not sure about the paths in README as I don't have a Windows machine, but happy to fix them if they're wrong. 🙂

I just checked, unfortunately they are variable:

E:\revery [master ≡ +1 ~4 -11 !]> esy x where Bin.exe
E:\revery\_esy\default\store\i\revery-5e49e93c\bin\Bin.exe

This makes it challenging to have a prescribed set of steps for running the app ☹️

The ideal scenario is to run via esy x Bin.exe - that way we don't care at all what the paths are, and let esy handle that. But this is blocked by #141 - we should be loading assets relative to the executable directory as opposed to the working directory.

Without #141 , this would make it cumbersome to run the app on windows. If we can fix #141 - we can both remove the buildsInSource and streamline our README.

An alternative to #141 is to do what esy did - add a bootstrap command that runs a node script that checks paths, and outputs a wrapper executable: https://github.com/esy/esy/blob/709dbef62ceec76b5347f8d85672abbe023c3c9c/package.json#L21

(But for revery, I'd view that as a short-term solution - we'll have to fix #141 no matter what, especially to be able to ship apps with it!)

@bryphe
Copy link
Member

bryphe commented Dec 27, 2018

With #141 - this is unblocked now! 🎉

@jchavarri
Copy link
Member Author

Thanks @bryphe !

Builds are passing so I'm gonna go ahead and merge this. I updated the readme to reflect the new approach to run the example. Heads up: there will prob be conflicts with #167.

@jchavarri jchavarri merged commit de7be4f into revery-ui:master Dec 27, 2018
@jchavarri jchavarri deleted the remove-buildsInSource branch December 27, 2018 23:31
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