-
Notifications
You must be signed in to change notification settings - Fork 37
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: limit view fields exposed to render function #138
Conversation
✅ Deploy Preview for anywidget canceled.
|
Codecov Report
@@ Coverage Diff @@
## main #138 +/- ##
=======================================
Coverage 97.14% 97.14%
=======================================
Files 8 8
Lines 420 420
=======================================
Hits 408 408
Misses 12 12 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Works for my use case.
packages/anywidget/src/widget.js
Outdated
|
||
/** | ||
* @typedef AnyWidgetModule | ||
* @prop render {(view: WidgetView) => Promise<undefined | (() => Promise<void>)>} | ||
* @prop render {(view: { model: WidgetModel, el: HTMLElement }) => Promise<undefined | (() => Promise<void>)>} |
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.
Is it odd that the variable is still called view
here?
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.
Ah yes, that's a good suggestion. Maybe a type like RenderContext
or RenderProps
would be better a more suitable type (variable names context
or props
respectively)
This would work for my use case, too. |
Description:
This PR addresses issue #137 by making changes to object exposed to user-provided
render
.Previous:
Proposed:
The key focus is on minimizing the API surface area by reducing the exposed fields to the user-provided render function. This change is expected to improve encapsulation and limit the potential for erroneous modifications.
In a review of current projects utilizing anywidget, no evident use-cases were identified that would be adversely affected by this change. It's worth mentioning that there's still a degree of uncertainty about how these changes will impact model (un)subscriptions within Backbone.
To-Do:
Request for Review:
I would like to invite reviewers to provide feedback, especially in areas related to Backbone's model (un)subscriptions. Any objections or observations on potential impacts are greatly appreciated.
cc: @maartenbreddels @juba @domoritz @kolibril13