feat: JS runtime #78
feat: JS runtime #78
Conversation
There was a problem hiding this comment.
praise: Was a really nice surprise as t how simple you managed to make this Keenan - really great job, I hope getting this deep into the guts of the lens codebase wasn't too painful.
praise: The change to the path variable is really nice :)
I have a handful of comments for you, but nothing too major - will hold off on approval until the Memory func thread has been resolved (I'll try to be prompt in giving any responses there)
Makefile
Outdated
| .PHONY: test\:js | ||
| test\:js: | ||
| $(MAKE) deps:test-js | ||
| $(MAKE) --no-print-directory -C ./host-go test:js |
There was a problem hiding this comment.
nitpick: Please fix the last line (end in line-break), same for host-go/Makefile
| @@ -47,12 +52,35 @@ func append[TSource any, TResult any](src enumerable.Enumerable[TSource], instan | |||
| // | |||
| // This is a fairly expensive operation. | |||
| func NewModule(runtime module.Runtime, path string) (module.Module, error) { | |||
There was a problem hiding this comment.
todo: Please document the new path behaviour and requirements on this public function.
host-go/engine/module/instance.go
Outdated
| GetData func() []byte | ||
| // Values written to memory will be made available to this module, however changes made by the | ||
| // module after this function has been called are not guaranteed to be visible to the returned io.Reader. | ||
| Memory func(offset int32) io.ReadWriter |
There was a problem hiding this comment.
question/suggestion: I dont fully understand why the offset param has been added - could you not just change the return type to ReadWriteSeeker and leave the func parameterless? That would let the caller remain responsible for seeking, and keep this (custom) interface simpler (allowing complexity and documentation to be offloaded to a std lib interface).
There was a problem hiding this comment.
I tried the io.Seeker route and it ended being more clunky than this approach.
Let me know if you are happy with the newly added module.Memory interface.
There was a problem hiding this comment.
Ah fair enough - new version looks good, glad you managed you make use of std interfaces for it even without ReadWriteSeeker - thanks :) I also wonder if Memory and BytesMemory might be useful outside of lens at somepoint.
host-go/engine/module/instance.go
Outdated
|
|
||
| // OwnedBy hosts a reference to any object(s) that may be required to live in memory for the lifetime of this Module. | ||
| // | ||
| // This is very important when working with some libraries (such as wasmer-go), as without this, dependencies of other members | ||
| // of this Module may be garbage collected prematurely. | ||
| OwnedBy any | ||
| } | ||
|
|
||
| type SliceReadWriter struct { |
There was a problem hiding this comment.
todo: Please document this and it's public functions.
Note: If you are able to remove the offset param from the Memory function, you might be able to find an existing 3rd party/std lib type that already implements ReadWriteSeeker for a byte[] and remove this type entirely
| var _ module.Runtime = (*wRuntime)(nil) | ||
|
|
||
| func New() module.Runtime { | ||
| // https://developer.mozilla.org/en-US/docs/WebAssembly/JavaScript_interface |
There was a problem hiding this comment.
praise: Thank you for including the links to the documentation, I might try and do this in my work.
suggestion: It might be nice to include a brief summary along side the link - it is more immediate to readers, and if the link dies or is moved we will still have some documentation. (same goes for all doc link locations)
Relevant issue(s)
Closes #79
Description
This PR adds a browser based JavaScript runtime to lens.
The runtime passes all tests within the
host-gopackage.Notable changes:
file://orhttp://https://prefixDatamethod of instance has been changed toMemoryruntimes.Defaulthas been added to return a default runtime for the current environment.Todo:
Tasks
How has this been tested?
make test:jsSpecify the platform(s) on which this was tested: