-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Port perspective-jupyterlab to TypeScript #2325
Conversation
e231625
to
53f81a4
Compare
/** | ||
* `PerspectiveView` defines the plugin's DOM and how the plugin interacts with | ||
* the DOM. | ||
*/ | ||
export class PerspectiveView extends DOMWidgetView { | ||
luminoWidget: PerspectiveJupyterWidget; | ||
perspective_client: PerspectiveJupyterClient; | ||
_pending_binary?: unknown; |
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.
these values are only ever assigned and copied around, and not used in any real way, so I think it's safe to leave them unknown
53f81a4
to
f33c77f
Compare
export type TableUpdate = { | ||
port_id: number; | ||
delta: | ||
| Array<Record<string, Array<string | boolean | Date | number>>> |
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 need to take another pass at these types, based on standup discussion they were right before
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.
Looks like delta
for this type actually no longer supports Array<...>
either, it is exclusively Arrow eg ArrayBuffer
Partly undoes 2d6e400 This change includes a restriction on the ipywidgets versions perspective declares that it supports. The plugin probably didn't work with ipywidgets <= 7 anyhow, since it calls `processLuminoMessage`, which in those earlier versions of ipywidgets was named `processPhosphorMessage`. see https://ipywidgets.readthedocs.io/en/8.0.7/migration_guides.html#phosphor-lumino and https://ipywidgets.readthedocs.io/en/8.0.7/migration_guides.html#updating-package-json if we want to support older ipywidgets, we should do something like ipywidgets docs describe
just kidding, but throwing on Dates is unacceptable. Gotta figure out how to make the types work or serialize the Date on the extension side
I don't think the types from earlier are correct now, they seem to correspond to arrays of structs of arrays, which I don't think perspective supports or uses (the constructor only accepts an array of structs for JSON anyway)
27d711a
to
ed99b9d
Compare
this.context.model.fromJSON(result); | ||
for (let [_key, val] of Object.entries(result)) { | ||
if (val instanceof Date) { | ||
throw new Error("TODO: Dates unsupported"); |
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.
this needs to be addressed. As of right now I don't recall why I added this loop to begin with - some cockamamie scheme in anger to satisfy the type checker? (this loop would be narrowing some of the types involved)
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.
Perspective outputs number
values in posix epoch time for columns of type date
and datetime
, ergo I think the fix for this is to remove Date
from the types of these methods in @finos/perspective
.
TODOs:
|
_psp: PerspectiveWidget & { | ||
theme?: string; | ||
}; | ||
_type: string; |
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.
type is one of "csv", "arrow", or "json" so can be constrained to string literal orring.
I would like some description of motivation if possible. One major motivation not to go back to typescript which won't be seen in this PR is the difficulty it presents when upgrading to new versions of Jupyter. Every time seems to present new and unique annoyances, with minor variances of the typescript compiler between what we use and what jupyterlab uses creating incomprehensible error messages. Given that the jupyter plugin is explicitly designed to NOT be used by anyone other than us, I don't think e.g. IDE convenience is a big motivation either. The jupyter plugin is not very large and not very actively developed, so there's not much devex benefit to be had either imo. |
Partly undoes 2d6e400
This change includes a restriction on the ipywidgets versions perspective declares that it supports. The plugin probably didn't work with ipywidgets <= 7 anyhow, since it calls
processLuminoMessage
, which in those earlier versions of ipywidgets was namedprocessPhosphorMessage
.see https://ipywidgets.readthedocs.io/en/8.0.7/migration_guides.html#phosphor-lumino
and
https://ipywidgets.readthedocs.io/en/8.0.7/migration_guides.html#updating-package-json
if we want to support older ipywidgets, we should do something like ipywidgets docs describe