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

refactor: internal ops are under Deno.core.coreOps #523

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

bartlomieju
Copy link
Member

This commit changes how ops defined in "deno_core"
crate are exposed to JavaScript code.

They are no longer added to "Deno.core.ops", but to "Deno.core.coreOps",
to make the distinction between them more obvious.

All ops that are required for embedders are now available under "Deno.core"
namespace.

This refactor uncovered a lot of cruft and technical debt accumulated over the
years - eg. there are several ops that are added unconditionally that are only
ever used in testing code. This poses a question, if "deno_core" should expose
some of the ops only when cfg(test) passes.

core/02_error.js Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

This file should probably be rolled into 01_core.js to avoid having to expose these APIs to Deno.core

@@ -241,10 +256,13 @@ pub(crate) fn initialize_primordials_and_infra(
}

/// Set up JavaScript bindings for ops.
pub(crate) fn initialize_deno_core_ops_bindings<'s>(
Copy link
Member Author

Choose a reason for hiding this comment

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

This function needs to be split into two separate functions - one for "core" ops and one for "ext" ops.

@@ -826,7 +826,7 @@ async fn test_promise_rejection_handler_generic(
function throwError() {
throw new Error("boom");
}
const { op_void_async, op_void_async_deferred } = Deno.core.ensureFastOps();
const { opVoidAsync, opVoidAsyncDeferred } = Deno.core;
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to verify it, but I believe deno relies on these ops as well

Comment on lines 127 to +130
.get(scope, key.into())
.unwrap_or_else(|| panic!("{path} exists"))
.try_into()
.unwrap_or_else(|_| panic!("unable to convert"))
.unwrap_or_else(|_| panic!("Unable to convert {path} to desired type"))
Copy link
Member Author

Choose a reason for hiding this comment

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

This funciton should probably be a macro and accept a token tree to print out the desired type nicely

Comment on lines +116 to +132
macro_rules! get_todo {
($type:ty, $scope: expr, $from:expr, $key:expr, $path:literal) => {{
let temp = v8_static_strings::new($scope, $key).into();
TryInto::<$type>::try_into(
$from
.get($scope, temp)
.unwrap_or_else(|| panic!("{} exists", $path)),
)
.unwrap_or_else(|_| {
panic!(
"Unable to convert {} to desired {}",
$path,
stringify!($type)
)
})
}};
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@mmastrac PTAL if this makes sense

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.

2 participants