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(core): allow specifying name and dependencies of an Extension #17301

Merged
merged 7 commits into from
Jan 8, 2023

Conversation

crowlKats
Copy link
Member

@crowlKats crowlKats commented Jan 7, 2023

This adds the ability to optionally specify a name and a set of dependencies (which are other Extensions) in an Extension, to provide a better error message when using the extension crates.

Unsure if additionally we should:

  • make specifying a name mandatory
  • panic if there are multiple extensions defined with the same name

@crowlKats crowlKats changed the title feat(core/extensions): allow specifying name and dependencies of an extension feat(core/extensions): allow specifying name and dependencies of an Extension Jan 7, 2023
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Unsure if additionally we should:

make specifying a name mandatory
panic if there are multiple extensions defined with the same name

I say yes - name should be a mandatory argument to Extension::builder() (eg. Extension::builder("deno_console") method and we should panic if there are multiple extension with the same name

core/runtime.rs Outdated Show resolved Hide resolved
ext/broadcast_channel/lib.rs Outdated Show resolved Hide resolved
@bartlomieju bartlomieju changed the title feat(core/extensions): allow specifying name and dependencies of an Extension feat(core): allow specifying name and dependencies of an Extension Jan 8, 2023
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, this should make embedding much easier

cli/ops/mod.rs Outdated Show resolved Hide resolved
Co-authored-by: Bartek Iwańczuk <[email protected]>
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.

None yet

2 participants