-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
Transpile (optional) TypeScript, support imports (package and relative) and schema #28
Conversation
Thanks @krassowski this is looking great!
Or we pull the repo when creating the Binder, in |
while we are left with non-pretty replacements of `require` with `await require`, maybe those could be performed in an `after` transformer in the future. The advantage of this approach is in lower maintenance burden (less to rewrite on each TypeScript major version bump) and a working implementation of exports, over quite complex custom transformer which was not fully functional for some edge cases. The disadvantage is that the transpiled code looks less like the original making debugging harder for plugin users.
This is now ready for review (and hopefully a merge). There was a major change in 0599fbc (followed up by cleanup in c1b011b): transpilation module target is now CommonJS as it turned out to be much easier to work with for full exports support (which is required for support of multiple files) - we just need to add Also, with newly added multi-file and
the first can be easily added in a follow up PR; the tsc compiler also supports react but it might require some more changes (I don't know yet). |
Ideally we would require a `.d.ts`, but for a dynamic playground I think it is fine to accept svg based on extension
I added support for React/tsx as as it was just a simple switch in tsc config. I don't plan to work on adding any more features. I tested most of the extensions and they just work :) There is a corner case of some extension examples using default import for React. It works fine if React is imported using: import * as React from 'react'; but fails when it is imported using a default import as in: import React from 'react'; Some extension examples do the former (e.g. kernel-messaging) and some the latter (e.g. custom-log-console). This comes down to |
Fixed in 580c723. |
This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there: |
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.
Great work @krassowski . This PR works for me (tested also with a few examples from https://github.com/jupyterlab/extension-examples). I have left a few comments.
let result; | ||
try { | ||
result = await pluginLoader.load(code, path); | ||
} catch (error) { |
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.
Would be good to show a dialog in case of success.
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.
Maybe. Ideally this would not be a dialog (which could be disruptive) but a notification. I guess we could defer until the notifications are in core.
await app.activatePlugin(plugin.id); | ||
// Unregister plugin if already registered. | ||
if (this.app.hasPlugin(plugin.id)) { | ||
delete (this.app as any)._pluginMap[plugin.id]; |
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.
The leaf/dependent plugins are not unloaded (see jupyterlab/lumino#278 (comment)). I guess this is ok for now
@@ -0,0 +1,107 @@ | |||
import * as jupyter_widgets_base from '@jupyter-widgets/base'; |
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.
Could this list of predefined modules be dynamic (maybe a feature for later?)
} catch (e) { | ||
showDialog({ | ||
title: `Plugin autostart failed: ${(e as Error).message}`, | ||
body: formatErrorWithResult(e, result) |
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.
My VS Code complains with Argument of type 'unknown' is not assignable to parameter of type 'Error'
. However, no issue when building from the CLI.
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.
Maybe VS Code is using a different or more version of typescript?
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.
You may have useUnknownInCatchVariables
enabled (or strict
). It is not enabled in main JupyterLab repo, but we could enable it here.
Co-authored-by: Eric Charles <[email protected]>
Looks like there is a small conflict to fix in Otherwise it works great on Binder, and we should consider getting this in soon! |
Co-authored-by: Jeremy Tuloup <[email protected]>
I would say ready to be merged. |
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.
Really nice, thanks!
Let's get this in an iterate 🚀 |
Starting a new release with this change. |
This is a follow up to #26 based on the discussion held in there. It is also a first step towards #25 and it relevant to #5. The ultimate goal is to make all the self-contained examples from jupyterlab/extension-examples just work without modification, including:
but excluding importing styles (ultimately we could handle those too).
In short, this PR uses TypeScript Compiler API to transpile the code into CommonJS understood by browsers (without type checking!) and TypeScript AST Transformer API with minor modifications:
require()
calls are replaced byasync require()
exports
are defined as an object on top of the module and returned in the body of new async function which serves the purpose of module executorThe
require
function is provided to the executor and depending on the module it returns:requirejs
is used; to avoid breaking the flow anawait
is used (the module loading is now done in n asynchronous function to accommodate that); as before, this will only work for AMD modules - and no further transpilation nor transformation is applied to the code retrieved byrequirejs
. To allow retrieval without explicitly passing the CDN URL, a new setting is added with a default pointing tohttps://cdn.jsdelivr.net/npm
.export default
isreturn
ed at the end, which gives us the plugin objectThe proposed changes are backwards compatible - if no
export
is found, the old loading mechanism will be used instead.The code can, but does not have to, be typed. Users who scrupulously annotate variables with types will be rewarded by better completer suggestions (at least on Binder where LSP is automatically installed).
Punchlist:
IPlugin.deactivate
,Application.registerPlugin
andApplication.deactivatePlugin
proposal lumino#278)require()
call to a CDN that they like the URL that is configured and accept the responsibility for loading arbitrary packages from web? This could be a settingallowCDN
with valuesuser-has-not-decided
,insecure-always
(always, but mention insecure to highlight the dangers),never
,(see There should be a way to verify subresource integrity #30)only-trusted-packages
Follow up items:
extension-examples
on those example; this is a big task and I would prefer to do this in a subsequent PRAdditionally:
and when pressed, it creates and opens a pre-populated minimal hello word plugin.