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

Improve nodejs module detection #2627

Closed
kidonng opened this issue Apr 22, 2021 · 7 comments
Closed

Improve nodejs module detection #2627

kidonng opened this issue Apr 22, 2021 · 7 comments
Labels
✨ enhancement A new feature implementation. ✅ existing workaround When there is an unofficial way to solve an issue and there is no fix in starship yet.

Comments

@kidonng
Copy link
Contributor

kidonng commented Apr 22, 2021

Feature Request

Is your feature request related to a problem? Please describe.

#2565 added deno module, which checks for a {mod,deps}.{j,t}s file, while currently nodejs module checks for *.{j,t}s. This produces a awkward result:

image

As the screenshot shows, I'm inside a pure Deno project, but the unrelated Node version is shown.


Beside my original motivation of opening this issue, I'd like to point out .ts is the file extension for MPEG transport stream as well. This is also common, for example, Deno (coincidentally, this issue is also about Deno) encountered a outage last month due to Cloudflare considering .ts files MPEG transport stream.

So my wish is if possible, Starship can check if any of the .ts files is plain text before deciding to show Node/Deno module (although it's unlikely you have a video stream named mod.ts, but who knows).

Describe the solution you'd like

A pure Deno project are unlikely to contain Node-specific files (node_modules, package.json, etc.), therefore if the directory contains {mod,deps}.{j,t}s while not Node-specific files, the Node version should be hidden.

Describe alternatives you've considered

Accepting the status quo of Node and leave it as-is.

@kidonng kidonng added the ✨ enhancement A new feature implementation. label Apr 22, 2021
@vladimyr
Copy link
Member

This reminds me of the node vs esy (OCaml's npm so to say) situation. It is handled by mutual exclusion conditions in the module code which we could apply here as well?

@vladimyr
Copy link
Member

So my wish is if possible, Starship can check if any of the .ts files is plain text before deciding to show Node/Deno module (although it's unlikely you have a video stream named mod.ts, but who knows).

FWIW, we could check file magic to ensure it is not a video stream but I don't think it's worth time doing. It is simply a too rare event to be handled gracefully.

@kidonng
Copy link
Contributor Author

kidonng commented Apr 28, 2021

This reminds me of the node vs esy (OCaml's npm so to say) situation. It is handled by mutual exclusion conditions in the module code which we could apply here as well?

Awesome! Looks like it's what we need exactly.

@kidonng
Copy link
Contributor Author

kidonng commented Apr 28, 2021

FWIW, we could check file magic to ensure it is not a video stream but I don't think it's worth time doing. It is simply a too rare event to be handled gracefully.

Well, I'm aware that the benefits may not worth it... It's not the main aim of this issue, just bringing up the idea though.

@sant123
Copy link

sant123 commented Sep 13, 2021

This is my workaround for Node.js projects

[nodejs]
detect_extensions = []

Since detect_files and detect_folders should be enough.

@andytom andytom added the ✅ existing workaround When there is an unofficial way to solve an issue and there is no fix in starship yet. label Jan 7, 2022
@kidonng
Copy link
Contributor Author

kidonng commented Mar 11, 2022

This is my workaround for Node.js projects

[nodejs]
detect_extensions = []

Since detect_files and detect_folders should be enough.

I wonder, if one day we can drop detect_extensions from nodejs module by default (or just leave Node-specific .{c,m}{j,t}s).

Node.js still remains the one true JavaScript runtime, let's see if Deno could become compelling enough to warrant its place.

@kidonng kidonng closed this as completed Mar 11, 2022
@matchai
Copy link
Member

matchai commented Mar 12, 2022

@kidonng That would be the end goal of #3673
Instead of checking whether you're in a nested directory of a project, which requires us to check for possible related extensions, we would just check the repo root for relevant project-wide files.

DrHyde added a commit to DrHyde/starship that referenced this issue Jun 14, 2022
DrHyde added a commit to DrHyde/starship that referenced this issue Jul 27, 2022
This reverts commit c1394fd.

This was a terrible way of doing this, there's got to be a better way!
davidkna pushed a commit that referenced this issue Jul 31, 2022
… from triggering (#4043)

* test that we can match a multi-part file extension such as in foo.tar.gz

* now we can match multi-part file extensions like on foo.tar.gz

* add a test that a !ext is a negative match and over-rides any positive match

* test that negative extensions that don't match any file have no effect

* fail the match if any negative extensions exist

* cargo fmt

I'm not happy with this, in particular it's made the structures of has_any_positive_extension and has_no_negative_extension look different, and the logic in is_match is harder to follow

* placate clippy

* documentation for multi-part extensions and negative extensions

* get rid of an unnecessary .to_string() and comment the necessary but weird-looking invocations of .to_string_lossy().to_string()

* tests for negative matching of files and folders

* fail the match is any negative files/folders match

* document file/folder negative matching; be less prolix

* suppress Nodejs if Deno files are present (#2627)

* Revert "suppress Nodejs if Deno files are present (#2627)"

This reverts commit c1394fd.

This was a terrible way of doing this, there's got to be a better way!
Indyandie pushed a commit to Indyandie/starship that referenced this issue Jul 26, 2023
… from triggering (starship#4043)

* test that we can match a multi-part file extension such as in foo.tar.gz

* now we can match multi-part file extensions like on foo.tar.gz

* add a test that a !ext is a negative match and over-rides any positive match

* test that negative extensions that don't match any file have no effect

* fail the match if any negative extensions exist

* cargo fmt

I'm not happy with this, in particular it's made the structures of has_any_positive_extension and has_no_negative_extension look different, and the logic in is_match is harder to follow

* placate clippy

* documentation for multi-part extensions and negative extensions

* get rid of an unnecessary .to_string() and comment the necessary but weird-looking invocations of .to_string_lossy().to_string()

* tests for negative matching of files and folders

* fail the match is any negative files/folders match

* document file/folder negative matching; be less prolix

* suppress Nodejs if Deno files are present (starship#2627)

* Revert "suppress Nodejs if Deno files are present (starship#2627)"

This reverts commit c1394fd.

This was a terrible way of doing this, there's got to be a better way!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement A new feature implementation. ✅ existing workaround When there is an unofficial way to solve an issue and there is no fix in starship yet.
Projects
None yet
Development

No branches or pull requests

5 participants