-
Notifications
You must be signed in to change notification settings - Fork 291
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
federated extensions, webpack sharing, deploying/configuring docs #58
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/jtpx/jupyterlite/6JKfpTnPYwWaSXeQrpGjdpd8b9Yc |
Thanks for getting this started! |
84f6972
to
07654a4
Compare
Trying this out locally:
Also changed the structure and boilerplate code so it follows the upstream example a bit more closely: https://github.com/jupyterlab/jupyterlab/blob/master/examples/federated/core_package/webpack.config.js |
Next steps:
so the lab settings manager can fetch all settings at once: It would be nice to be able to just copy the prebuilt extension assets to |
Thank you so much for looking into this. I'll check it out soon... the delta from my broken stuff should be enough for me to keep moving forward.
Yeah, sounds like table stakes for getting this in. Humans should not have to write anything other than the one JSON file that seeds pageinfo, etc. so this is just another part of that, i guess... |
Nothing to concretely comment on, but the duplication between classic/lab definitely points to us perhaps wanting a |
Yeah that sounds good.
And for now we could limit the scope to Python driven installs, which sounds the simplest so we could copy straight from |
Oh, yes, indeed. Nobody benefits from us building another package format. So in another nuts-and-bolts, hopefully-testable diagram, I think we want two paths:
Both would end up fully realizing in |
Further, I think we have to plan for being able to deploy servlite
extensions _as_ federated extensions, and downselect them based on some
metadata at setup time... We can't maintain (and make people go use)
_another_ build chain, and then figure out where to upload it, etc.
It should be possible to work against a local one, however, without
packaging, ideally live deployed into the app with "labextension watch"...
We can monkey patch is we must, but I think all the switches are available
some way or another.
However, we don't need to solve that on this pr! Well already be able to do
lots of good demos with this, modulo the things we've called out (which
I'll hoist to the checklist)
|
I am making progress here, but not necessarily in the direction i was expecting:
This whole yak 💈 is to demonstrate being able to have a single So I reckon we add |
Thanks, this is looking great!
Yeah absolutely, need to give some more thoughts to this. Also in the case where third-party packages want to distribute both a
Sounds good 👍 I think we could also have a list of curated extensions that work well, or just some that we like to use. |
app/config-utils.js
Outdated
/** | ||
* TODO: consider better pattern for invocation. | ||
*/ | ||
await main(); |
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 this might give a top level await error on some browsers.
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.
yeah, i just wanted something that didn't webpack for this stage. as mentioned, we likely want this for "in-app" stuff too, so it probably needs to become a package with an entry point in the top-level webpack.
We also need to do more work on cache-busting, getting that [hash]
back into file names, as I've noted on RTD it's pretty easy to get in a bad cache state. there's probably some more stuff we can fiddle with to get more-reproducible chunks.
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.
So top level await seems to be supported by Chromium 90 and Firefox 89.0b7 (developer edition). But not in Firefox 88.
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.
Actually do we need the await here? Or could this be changed to the following (at least for now):
void main();
Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>
.eslintrc.js
Outdated
@@ -45,7 +45,7 @@ module.exports = { | |||
{ avoidEscape: true, allowTemplateLiterals: false } | |||
], | |||
curly: ['error', 'all'], | |||
eqeqeq: 'error', | |||
eqeqeq: ['error', 'always', { null: 'ignore' }], |
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.
Do we still need the null: 'ignore'
part? Or was is for testing?
It would be nice to stick to ===
checks in general, but maybe this was added for a specific reason?
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.
strict equality of null
and undefined
is not consistent, whereas if you let the browsers Do Their Casting Thing, it comes out correctly... in my experience. Indeed, I prefer the behavior where it complains if you ever try to strict equal null
or undefined
.
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.
So do you think we should keep that ignore
clause, or leave it like it was before?
It might sound like a cosmetic thing mostly, but I was thinking we could try to follow the JupyterLab TypeScript Style Guide to make it easier for folks to switch between code bases and contribute to these projects. It doesn't explicitly say anything about strict equality, but I guess it would try to follow the JupyterLab eslint / prettier config when that makes sense.
packages/settings/src/settings.ts
Outdated
name: STORAGE_NAME, | ||
description: 'Offline Storage for Settings', | ||
storeName: 'settings', | ||
version: 1 | ||
}); | ||
|
||
export async function getAll(): Promise<{ settings: IPlugin[] }> { |
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 the settings are not updated on the RTD preview for this PR, for example when switching the theme or moving tabs to the left and right area: https://jupyterlite--58.org.readthedocs.build/en/58/_static/lab/index.html
Although refreshing the page does pick them up. So maybe it's related to the new getAll
"merging" logic.
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.
yeah, gross. needs test #6 lol
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.
Yeah absolutely.
Just pushed c0bff5f in the meantime which should fix it.
It adds the network requests back to fetch all.json
and the federated extension schemas too. But it would fine to take a look at it again in a separate PR if we want to optimize this.
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 should also handle editing settings for a federated extension via the settings editor.
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.
yeah, settings editing works now that the isFederated
check is accurate
That's a whacking good idea. quick sketch:
As for that dumb file:
No complaints there! Indeed, #6 should be testing that (probably deployed locally with e.g.
Well... on Jyve, i paid the 🐒 price to make stuff work in both full lab and the offline thing, and it was a huge hassle... but i'd definitely want to be able to use things like What we can't do is introduce a new package build chain: it's gotta build with I'd see hoisting |
Using
That would be ideal yes. Just need to check whether we can keep that workflow given that the jupyterlite server app is (for now at least) a separate Lumino application. |
I strongly desire being able to ship jupyter tools with |
packages/settings/src/settings.ts
Outdated
*/ | ||
export async function getFederated(id: string): Promise<IPlugin | undefined> { | ||
const [ext, plugin] = id.split(':'); | ||
let federated: 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.
oops, i think this comes back as a {name, load, extension, style}
... will update...
packages/settings/src/settings.ts
Outdated
} | ||
|
||
/** | ||
* Get all the settings | ||
*/ | ||
async getAll(): Promise<{ settings: IPlugin[] }> { | ||
return Private.getAll(); |
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.
so... these will definitely not pick up federated stuff at present...
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.
looked into this a bit... the problem is there is no enumeration of ["@org/pkg:plugin", ...]
anywhere in the metadata. When we get to the "for real" automation tools, we may wish to hoist that information as well, as we would have the contents of the canonical schemas
directory available, and could make that available to jupyter-config-data
.
it could also be that the convention of the directory structure isn't actually enforced... but i think we'll just have to found out once we have the automation to start trying out a bunch of a extensions.
Yeah want to circle back on that... Ideally it would be in ts, as part of a
single build, with a cache-busting target, as if you get a bad cache of
that and things change, it's not going to go well.
Then each of the Index.html would also have to point at the cache-busted
script, which complicates matters, but it's worth it.
Also want to hoist the schema to ts types.
|
@bollwyvl I just pushed a couple of minor tweaks to:
Otherwise I think it looks really good for a first iteration on federated extensions! We can merge it as is and iterate in follow-up PRs and issues. |
Sure, sounds good if you're feeling it. We should hoist the TBD from this
to new issues if not already done... Will look later today
|
This is really great stuff, thanks again Nick! |
References
Code changes
index.html
filesall.json
file on build:lab/extensions
@jupyterlite/top
(orsite
or something) that carries and abstracts some of the boilerplateUser-facing changes
Deployer
<app>/extensions
(as copied from$share/labextensions
).py
and/or.js
script to update variousall.json
filesEnd User
Backwards-incompatible changes
?