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

api improvement #1470

Closed
wants to merge 2 commits into from
Closed

Conversation

dev-ardi
Copy link
Contributor

@dev-ardi dev-ardi commented Apr 22, 2024

This addresses #1467

This new API design has the following advantages

  1. It makes it sound to initialize isolates
  2. It uses builder pattern to create a platform. This is better because it doesn't require users to explicitly think about uncommon options, and better documents existing options in case that users might need them.
  3. It makes it impossible at compile time to create an isolate without having initialized the platform
  4. It's uncommon to create an isolate with options so make the default way of creating them not take any options.
  5. This leads to nicer code and it's more rusty:
let isolate = v8::V8Options::new()
  .build() // This hints that it's builder pattern and that there are more options that could be used, but doesn't forces them to know them.
  .isolate();

@CLAassistant
Copy link

CLAassistant commented Apr 22, 2024

CLA assistant check
All committers have signed the CLA.

@mmastrac
Copy link
Contributor

I think it's better if we address #1467 separately from the builder pattern for init. While I don't think it's a bad thing to readdress isolate initialization, that's a totally different topic.

@dev-ardi
Copy link
Contributor Author

The idea is there. Feel free to do with it what you will.

@dev-ardi dev-ardi closed this Apr 23, 2024
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.

3 participants