-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
feat: native plugins #3372
feat: native plugins #3372
Conversation
fef9c0c
to
661e5a4
Compare
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 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
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 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
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.
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 🚀
.github/workflows/ci.yml
Outdated
@@ -129,6 +129,7 @@ jobs: | |||
- name: Test debug | |||
if: matrix.kind == 'test_debug' | |||
run: | | |||
cargo build --locked -p test_native_plugin |
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.
Can this step be somehow integrated into cli/tests/integration_tests.rs
flow?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
LGTM - Thank you for the long and hard work of getting this in @afinch7 !
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