Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature/using component #678
Feature/using component #678
Changes from 6 commits
4349443
d40ce20
b50947c
925d910
9975e45
277ab71
c248d38
f9900d0
3a627bd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thinking aloud: should we maybe not hand over a file path but rather the wit definition (file content) itself?
maybe the wit content doesn't come from a file but from - i don't know - a network request or was generated somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. as it turns out rust code annoyingly needs a path passed to it, but it doesn't have to actually be a path that exists. I think it uses it to produce errors perhaps. In any event, if we change it to pass the wit contents as a string we have to make up a path somehow. Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking I will just have the
wit
option just mean a string containing the wit content, and add awit_path
orwit_file
option to specify the file(s). In the future I'd also like to addcomponent
orcomponent_bytes
options to pass in the binary, which I think I can also parse with a little more codeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't wait for imported function support! :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, definitely high on my list of next things.
Check warning on line 96 in lib/wasmex/native.ex
GitHub Actions / OTP 25.2 / Elixir 1.15.8
Check warning on line 96 in lib/wasmex/native.ex
GitHub Actions / OTP 25.2 / Elixir 1.15.8
Check warning on line 96 in lib/wasmex/native.ex
GitHub Actions / OTP 26.2 / Elixir 1.15.8
Check warning on line 96 in lib/wasmex/native.ex
GitHub Actions / OTP 26.2 / Elixir 1.15.8
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking at these
unwrap
calls: When/how would these trigger? can they be triggered through user input? Wouldn't be great if we crash the BEAM just because someone chose a weird field name 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you care only about functions for this PR, but is it an option to return all exports of the module instead?
similar to
module_exports
for regular wasm modules.We would share a common/similar API (which I like for consistency) and having that additional info will surely help us later and we don't have to break any API for those changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a look at module exports and see if we can consolidate, agree having a consistent api would be best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be nifty if we could also return the types (params types and return types) for nicer introspection/debugging/tooling later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also to prepare for elixir types (whenever they become available) - would be ace if wasmex could created typed interfaces/function definitions right away
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, i did this way for expediency in getting something working we need a better API to get back data from wit parsing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we touched this topic in a private conversation before, but let's bring this discussion here :D
I love the idea! Calling component modules must be easy and automated and what you did here is much much better than anything we already have in wasmex. I'm not 100% certain how to do it though and would love your thoughts!
I was (and still am) internally debating whether we should use macros (as you did here) or rather have code generation. Let's try to collect a few pros/cons and see what's better. (please add your thoughts - I'm not decided, and we could even end up doing both)
Macro
wit
and you're goodCode Generation
mix phx.gen.auth
, andigniter
whereas macros are were heavily used in ruby codebases and José consciously decided against their magic when designing Elixir in many places)Conclusion
So. I don't know. I guess it's a trade off as all things in life. Coming from a corporate thinking (my day job ruined me) code generation looks sweet. it's more explicit and less magic. code reviews and manual extensibility seems to be favoured and it looks to vibe with what (I think) Elixir intends to be. But macros are so much easier to set-up.
What are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it was a macro, would we still have compiled-time checking of the name of functions and number of arguments passed? I think that’s the main thing I would want, apart from type-checking.
If I change my WIT file, and the Elixir compiler alerts me via the macros changing then that’s a good UX imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I know or entirely understand your questions @RoyalIcing but in my experiments, things like completion in iex still work so there appear to be "normal" behaviour for the macro generated functions, since macros are expanded at compile time. I'm not sure this is a one size fits all answer here, so I kind of lean towards eventually supporting both options. That said, I think I'll continue on with the macro approach for the moment as I flesh things out as its an approach I'm pretty familiar with vs learning Igniter (which does sound fun but only so much time in day etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@superchris My thinking is round what happens when the WIT changes. If the WIT is consumed by the macros then that would lead to a compile time error for most changes, which is what you’d want for a good developer experience.
If you went with the code generation I’d want some compile-time check that verifies the WIT and generated code still agree anyway. So for me it would really come down to whether being able to read the interface as Elixir code would be helpful or not. If you had code generation would you ever want to alter that code? I’m thinking probably not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh! what a discussion here and on bluesky! considering the macro is a very thin wrapper right now, I'd say let's merge it!
we maybe want to expand this feature into more than that which might end up being code generation (e.g. to support an elixir module having only a sub-set of the exported functions implemented) but not now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love it! would be great to see imported functions being support soon 🙌
oh and is it possible to move this module just above the test where we use it? (like an inlined module). this would make the test way easier to read IMO (since we don't have to jump files)