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
core: factor out EsIsolate from Isolate #3613
Conversation
|
||
pending_dyn_imports: FuturesUnordered<StreamFuture<IntoStream<DynImport>>>, | ||
dyn_import: Option<Arc<DynImportFn>>, | ||
waker: AtomicWaker, |
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.
Was AtomicWaker already part of deno_core::Isolate?
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.
Yes, it's used in two situations:
a) more ops to poll
b) more dyn imports to poll
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.
This doesn't need to be addressed now but feel like we should avoid tying deno_core to tokio. The AtomicWaker feels like it's unnecessarily growing the abstraction layer of deno_core.
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.
AtomicWaker
is part of futures
crate
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 think this separation is good.
@ry I'm not sure about the name though 🤔 |
@bartlomieju EsIsolate? I think it's ok. |
Alright 👍 |
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
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.
Some nits please
This PR factors out all module related stuff from
Isolate
to newEsIsolate
.Next step would be to refactor
core/modules.rs
by merging it's structs withEsIsolate