-
Notifications
You must be signed in to change notification settings - Fork 9
Allow instantiation of multiple notebooks inside a single scope #40
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
Conversation
mbostock
left a comment
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.
Makes sense; we should generalize this to be more object-oriented. I think the pattern should probably be to instantiate something, an object that owns the runtime and the main module, and then call define on the instantiated object as a method, rather than passing the main module through to define.
|
That would be even better, was going with a minimal PR. |
5004fe2 to
0e51b2c
Compare
|
Updated as suggested (included the ability re-define or undefine a cell, as per our previous discussion) |
mbostock
left a comment
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.
Thanks for incorporating my feedback. This looks useful.
For backwards compatibility, I think we’ll want a defaultInstance and retain the existing define, main, runtime symbols (on top of the defaultInstance, which doesn’t need to be exposed directly). We can deprecate these but I’d like to maintain compatibility with the current release.
mbostock
left a comment
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’d also prefer to keep the redefining and stateById logic out of this PR and leave that in userland for now. You should still be able to implement it as described in #26 (comment).
src/runtime/define.ts
Outdated
| this.main.constructor.prototype.defines = function (this: Module, name: string): boolean { | ||
| return ( | ||
| this._scope.has(name) || | ||
| this._builtins.has(name) || | ||
| this._runtime._builtin._scope.has(name) | ||
| ); | ||
| }; |
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.
With the defaultInstance approach, this mutation could be done statically. (But really we should move it to the Runtime itself… Future work!)
0e51b2c to
7ca52cf
Compare
7ca52cf to
6f97e9f
Compare
Signed-off-by: Gordon Smith <[email protected]>
6f97e9f to
1114cd6
Compare
|
Superseded by #49. |
FWIW I am creating notebooks at runtime (rather than build time) and having a single runtime/main is problematic.