Skip to content
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

Copy absolute path with first slash in the file browser #15168

Merged
merged 3 commits into from
Nov 2, 2023

Conversation

pauky
Copy link
Contributor

@pauky pauky commented Sep 28, 2023

References

Follow-up of #14842
There is no first slash when coping a absolute path.

Code changes

Change PathExt.json to posix.json.

User-facing changes

None.

Backwards-incompatible changes

None.


Edit to specify how it relates to 14842

@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@@ -65,6 +65,7 @@ import {
import { find, map } from '@lumino/algorithm';
import { CommandRegistry } from '@lumino/commands';
import { ContextMenu } from '@lumino/widgets';
import { posix } from 'path';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work as the code is running in the browser, not in a Node runtime.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function PathExt.join was wrapped by it, and it can be execed in browser.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it cannot be used in browser because browsers do not implement Node runtime. You would need to add a dependency on path-browserify, which is what the @jupyterlab/coreutils package which implements PathExt.join does:

"path-browserify": "^1.0.0",

However, ideally this implementation detail and dependency would not be propagated to filebrowser-extension, and either a new argument would be added to one of PathExt methods, or a new method created altogether and used in here.

@fcollonval fcollonval added this to the 4.1.0 milestone Sep 29, 2023
@fcollonval fcollonval added the bug label Sep 29, 2023
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @pauky

@krassowski krassowski changed the title fix(filebrowser-extension): copy absolute path with first slash Copy absolute path with first slash in the file browser Oct 28, 2023
@krassowski krassowski merged commit d7d0913 into jupyterlab:main Nov 2, 2023
78 of 80 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants