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

All of an op's code should live together #2180

Closed
ry opened this issue Apr 22, 2019 · 3 comments
Closed

All of an op's code should live together #2180

ry opened this issue Apr 22, 2019 · 3 comments
Assignees

Comments

@ry
Copy link
Member

ry commented Apr 22, 2019

After we've replaced flatbuffers (#2121), I want to refactor how we define ops. Ideally the javascript and rust code would live next to each other. Example dir structure:

//ops/read_file.rs # contains unit tests, resource definitions, and the op itself
//ops/read_file.ts
//ops/read_file_test.ts

Currently adding an op means touching a lot of files spread around the tree.

An extreme case of this design would make each op into a standalone crate which could be installed separately, so an embedder could pick and choose the functionality they wanted.

@ry ry added this to the future milestone Apr 22, 2019
@ry ry self-assigned this Apr 22, 2019
@ry ry changed the title All of an ops code should live together All of an op's code should live together Apr 22, 2019
@zekth
Copy link
Contributor

zekth commented Apr 23, 2019

Is a sub folder for each op is revelant? Due to the number of ops it could become a mess. Just an idea.

//ops/read_file/read_file.rs

Also making each op in a crate would be a bit overkill and time consuming for any update no?

@afinch7
Copy link
Contributor

afinch7 commented May 20, 2019

It might be a bit more manageable if we group ops together I.E. read, write, copy, etc.. in one crate. I still think the following things are going to be the most problematic:

Generating snapshots at compile time.

Exposing a list of required resources and respective source code would be easy to achieve after compile time I.E. some_op::code = include_bytes!("source/file.ts").

Exposing a complete list before compiling would be more difficult if we want to avoid arbitrary external definitions for these things.

In an ideal world including a op would be simple I.E. Worker::new([deno_op_read::op, deno_op_write::op]).
In reality it might be a bit more complicated I.E. def_deno_op!(Worker, [deno_op_read::op, deno_op_write::op]).

Avoiding circular dependency structures when defining the dispatch function for a op.

I.E. deno_op_copy <- deno_cli (for ThreadSafeState) <- deno_op_copy

I'm not sure how rust handles circular dependencies, but if other languages are anything to go by it will be messy if it works at all.

We may need other crates to define capability specific traits for dispatch contexts and implement them for deno_cli::ThreadSafeState.

trait DenoDirContext {
  fn get_deno_dir(&self) -> DenoDir;
}
impl DenoDirContext for ThreadSafeState {
  fn get_deno_dir(&self) -> DenoDir {
    self.dir
  }
}

This would make deno more modular for embedding.

Maintaining type safety in the compiler.

This is really just a result of the first one. Getting what we need to generate types at compile will be harder without resorting to some arbitrary include list.

I figure it's already been thought about, but I figured I would mention how much this change looks like the stepping stones to modular bindings like node. @ry Any insight what you want to avoid in a binding system?

@kitsonk
Copy link
Contributor

kitsonk commented Nov 5, 2020

This is very dated and we have been through several iterations of approach. As it stands, this issue is generally un-actionable, so closing.

@kitsonk kitsonk closed this as completed Nov 5, 2020
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

No branches or pull requests

4 participants