Skip to content
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: align plugin api with Extension #10427

Merged
merged 21 commits into from
May 7, 2021
Merged

feat: align plugin api with Extension #10427

merged 21 commits into from
May 7, 2021

Conversation

eliassjogreen
Copy link
Contributor

Continuation on #9850. This pr aligns the plugin api with the new runtime extensions introduced in #9800. I have not yet limited any of the access to the Extension struct so currently they are the exact same (Except js, and middleware support which probably would not be needed nor used. Reason for not implementing them is that they need access to the JsRuntime which op_open_plugin does not have). The Extension struct should probably be split into a few traits (JsExtension and MiddlewareExtension) or a new DynamicExtension struct could be introduced.

cc @crowlKats @AaronO

@eliassjogreen eliassjogreen marked this pull request as ready for review May 3, 2021 12:22
@lucacasonato
Copy link
Member

@eliassjogreen Could you merge with master?

@eliassjogreen
Copy link
Contributor Author

eliassjogreen commented May 6, 2021

Deno.core.opSync and Deno.core.opAsync should probably be exposed outside of core in the normal Deno namespace, right?

@ry
Copy link
Member

ry commented May 7, 2021

Deno.core.opSync and Deno.core.opAsync should probably be exposed outside of core in the normal Deno namespace, right?

Why's that? These are not methods we generally want user code to call. I suppose the distinction is blurred in the case of plugins.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks @eliassjogreen for giving the plugin API some love.

@ry ry merged commit 4ed1428 into denoland:main May 7, 2021
@AaronO
Copy link
Contributor

AaronO commented May 7, 2021

Deno.core.opSync and Deno.core.opAsync should probably be exposed outside of core in the normal Deno namespace, right?

Why's that? These are not methods we generally want user code to call. I suppose the distinction is blurred in the case of plugins.

One possible workaround would be to have openPlugin return { opSync, opAsync, ... } to limit exposure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants