-
Notifications
You must be signed in to change notification settings - Fork 20
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
Import Web Assembly support from snapchat's djinni fork #157
base: main
Are you sure you want to change the base?
Import Web Assembly support from snapchat's djinni fork #157
Conversation
9d18164
to
9ea2a7b
Compare
Wow, that's a pretty fantastic development! Just a question: After looking back at the Python addition, I see that it is now 80% to 90% done but then slowed down. |
Yes, I think we need some kind of definitive sample for this to be moved out of draft status. I haven't checked what there is in the snapchat repository, yet. I understand your concerns about abandoned platforms, but (at least for me) web assembly is pretty essential. I think that, given the state of the support on the snapchat fork, it should be possible to get the web assembly support here quite usable without too much pain. Fingers crossed. Next up the support lib, I guess. |
3027080
to
e0c0dce
Compare
src/it/resources/expected/all_datatypes_json/cpp-headers/my_flags.hpp
Outdated
Show resolved
Hide resolved
801ded6
to
76ef1b2
Compare
One of my general concerns with this PR is how many changes seem be outside of the new generator code, as in theory, adding a new generator should change little of the existing code other than extending the places to enable the new output. E.g. Ive started working on a swift and kotlin generator and so far haven't had to make any changes to existing code other than adding the options and extra bools to enable them during testing |
I agree, we need to check that all changes are related to the WASM part and not sneak in via any other Snapchat additions that might change behavior. On the other hand, if the expected files from the tests are unchanged, nothing should have changed (or our tests are incomplete). So we should have some safety net in place, |
I've been trying to reduce these changes. I think all that remains is the |
d77aee4
to
d78420d
Compare
I am sorry, but looking at my calendar, it is very likely my review must wait until the Christmas holidays. |
ba6c5bb
to
e5e0995
Compare
After thinking about this PR for one year, I tend to not use it. In hindsight, it would have been beneficial to keep the original two language bindings, Objective-C and Java/JNI, and focus only on those. And add anything else in a djinni2, based on different ideas and technologies. That is probably what should happen for this project. |
I can completely understand that. I think that PyDjinni has done a nice job of making language bindings a bit more modular, but with the original Djinni it is difficult to do so. While I like quite a bit how this project is organised and operated, it's a pity to me that it will continue to diverge from Snap's fork and not support web. If one could only choose two platform languages, then I agree those two should be for Java and Android. For a third platform, personally I think web/webassembly makes the most sense - but in any case, ifi need web, I can just use Snap's fork. By all means, feel free to close the PR. |
The question is, @mutagene , would you be willing to join as maintainer and help maintain the Webassembly implementation? And commit to that. |
I felt quite inspired by @eakoli 's latest contribution. I'm not sure there's the appetite for this, but at least on my projects, if I'm using djinni in the future it's hard to imagine not wanting the possibility of a web assembly (and flutter?) target. This is quite a naive attempt to steal that from the snapchat djinni, and without the lib-side work (which I haven't done yet), I'm not sure it even works. Anyway, the way these things go I may have time to work on it some more and I may not, so I thought I'd make the (draft) PR now and then worry about the rest later.