Repository navigation
feat: native plugins - #3372
feat: native plugins#3372
Conversation
fef9c0c to
661e5a4
Compare
bartlomieju
left a comment
There was a problem hiding this comment.
There are a few things I have questions about, but I believe this might be very solid MVP for native plugins;
a) it's built on ops
b) this is the simplest iteration, doesn't expose state to op dispatcher in Rust, we can iterate on API safely
4cdeeb2 to
51910ee
Compare
ry
left a comment
There was a problem hiding this comment.
I like that this is only +400 lines
7df0cbd to
1fc95be
Compare
1fc95be to
112622d
Compare
|
For some reason |
45f301d to
cb94110
Compare
5c796ea to
62c126c
Compare
bartlomieju
left a comment
There was a problem hiding this comment.
It is very solid PR, especially because all changes introduced in it are minimal - it allows to iterate on the API before 1.0 release. It's well aligned with our ops APIs and has minimal impact on current architecture 👍 I'd love to see this landed to give users chance to use it and break it 🚀
| - name: Test debug | ||
| if: matrix.kind == 'test_debug' | ||
| run: | | ||
| cargo build --locked -p test_native_plugin |
There was a problem hiding this comment.
Can this step be somehow integrated into cli/tests/integration_tests.rs flow?
There was a problem hiding this comment.
I tried to add a single test to test_native_plugin, but it still doesn't build automatically when running cargo test --all-targets. Not sure if there may be some other simple workaround here.
There was a problem hiding this comment.
We should figure out a way to test this without making changes to ci.yml... I'm looking into it.
f780f53 to
0bb50b3
Compare
* Rename `allow_native`/`allow-native` to `allow_plugin`/`allow-plugin`. This makes more sense with the primary public api function being `openPlugin`. * Fully intagrate the newly added permission type `plugin`. * Add docs to `Deno.openPlugin`. * Generate op names for native plugin ops that avoid potential collision. Lastly this commit adds a delayed future to native plugins async test op. This should properly test the function of contexts/tasks/wakers in async native plugin ops. This would not function correctly with futures 0.1, and thus was a major blocker. This test proves this issue no longer exists in this implementation. The most analogous problem I can think of here is loading two copies of the same TS lib: the share the same type thus are interoperable, but don't share the same module local data(closest comparison to TLS used to store current task in futures). This isn't a problem anymore, since futures now pass `Context` values directly during calls to `poll` with the new API.
|
I've merged this branch with master and moved a few things around. I think the plugin infrastructure shouldn't depend on cli, so the tests shouldn't be in cli. I'll try to do that now... |
|
So.. I'm still having the same trouble you were having, @afinch7, with getting the plugin to build for the test. I guess moving the test_plugin crate to the root didn't help. I've done some superficial clean ups. I'm pretty much ready to land this if we can get it to go green somehow. |
| fn basic() { | ||
| let mut build_plugin_base = Command::new("cargo"); | ||
| let mut build_plugin = | ||
| build_plugin_base.arg("build").arg("-p").arg("test_plugin"); |
There was a problem hiding this comment.
It's annoying that it's not built already when the integration tests run. I wish I knew some cargo expert who could advise us here.
Now that we are using futures 0.3 I should be able to finish this.
Api will look a little different this time to reflect some of the refactors that have occurred since #2385.
I'm targeting something like this on the plugin side:
and something like this on the typescript side:
refs #3366 #2473