Skip to content

fix: don't show "start a build" if the file is in node_modules #472

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jun 29, 2022
Merged

fix: don't show "start a build" if the file is in node_modules #472

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc6002b
do show prompt if in node_modules
Jun 29, 2022
a6f8f07
fix
Jun 29, 2022
974cc72
reset ci
ah-yu Jun 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions server/src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ let projectsFiles: Map<
// automatically, if there's no build currently running for the project. We
// only want to prompt the user about this once, or it becomes
// annoying.
hasPromptedToStartBuild: boolean;
// The type `never` means that we won't show the prompt if the project is inside node_modules
hasPromptedToStartBuild: boolean | "never";
}
> = new Map();
// ^ caching AND states AND distributed system. Why does LSP has to be stupid like this
Expand Down Expand Up @@ -188,7 +189,7 @@ let openedFile = (fileUri: string, fileContent: string) => {
openFiles: new Set(),
filesWithDiagnostics: new Set(),
bsbWatcherByEditor: null,
hasPromptedToStartBuild: false,
hasPromptedToStartBuild: projectRootPath.includes("node_modules") ? "never" : false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this just string search?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any risk one puts the file in this_is_not_node_modules_carrots or is that too pedantic?

Copy link
Collaborator

Choose a reason for hiding this comment

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

One could make it includes("/node_modules/") and we should be as safe as we can be.

Copy link
Contributor Author

@ah-yu ah-yu Jun 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@zth but it will fail on windows

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right. But looks like you accounted for that in your fix.

};
projectsFiles.set(projectRootPath, projectRootState);
compilerLogsWatcher.add(
Expand Down