-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
Extract out ICollaborativeDrive to @jupyter/collaborative-drive
#353
Extract out ICollaborativeDrive to @jupyter/collaborative-drive
#353
Conversation
4bb9690
to
b0b5c7a
Compare
cf02af4
to
de0c62e
Compare
de0c62e
to
13fe0c4
Compare
Should there be a new |
eb1976f
to
b0f2621
Compare
I'm wondering if this failure:
is caused by the fact that |
484d489
to
95c62d1
Compare
95c62d1
to
cece00b
Compare
581484c
to
32a3e5a
Compare
@krassowski What do you think about getting this in before releasing v3? |
@jtpio do you have the rights to publish |
@@ -0,0 +1,55 @@ | |||
{ | |||
"name": "@jupyter/collaborativedrive", |
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 we should rename the package to the following, so it reads better, before publishing it?
"name": "@jupyter/collaborativedrive", | |
"name": "@jupyter/collaborative-drive", |
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.
Fine with me 👍
0997374
to
d08c6a8
Compare
d08c6a8
to
6cd68c2
Compare
I'm planning to merge today. |
Thanks @davidbrochart. I'll try to have another look today. |
@@ -71,6 +71,7 @@ jobs: | |||
pip install "jupyterlab>=4.0.0,<5" | |||
pip install -e . | |||
jlpm | |||
jlpm build |
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.
Why do we need it 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.
I don't remember the reason, I'll remove it to see what happens.
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.
See here:
src/yprovider.ts:6:35 - error TS2307: Cannot find module '@jupyter/collaborative-drive' or its corresponding type declarations.
6 import { IDocumentProvider } from '@jupyter/collaborative-drive';
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
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 it will be fine once @jupyter/collaborative-drive
is published?
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, maybe I can help publish a placeholder version to see if it helps.
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.
It looks like a version with the previous name was also published earlier: https://www.npmjs.com/package/@jupyter/collaborativedrive
Maybe it can be removed from npm
so it's less confusing?
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.
Now I'm confused, who published that? Do we use it somewhere?
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.
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 guess you published it to see if that would help fix the CI?
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, I don't remember.
@jupyter/collaborative-drive
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.
Let's rename the folder from:
packages/collaborativedrive
To
packages/collaborative-drive
So it's consistent with the package name?
Same error:
I guess the published package is useless without code. I'll add |
What do you mean by "without code"? The package was published from this branch directly. |
Oh my bad, yes the code is there. Then I don't know what is wrong. |
Having to add |
@@ -2122,10 +2123,22 @@ __metadata: | |||
languageName: unknown | |||
linkType: soft | |||
|
|||
"@jupyter/collaborative-drive@^3.0.0-beta.6, @jupyter/collaborative-drive@workspace:packages/collaborative-drive": |
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 line looks different than for the other packages.
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.
Good catch, let's try to make it similar.
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 it's Yarn that adds @jupyter/collaborative-drive@^3.0.0-beta.6
.
c380720
to
dc8f7a1
Compare
Actually yes, there is no code here. |
c8514d5
to
4c685ca
Compare
I published |
In the end, I just think that |
I'll take the risk to merge this PR and release jupyter-collaboration v3.0.0beta7. We can still revert the changes and release v3.0.0beta8 if it doesn't work. |
Arf, I wanted to help you more on this yesterday but didn't have time. At the very least it would have been great to leave a TODO, or open an issue... Maybe check the |
See #352 (comment).