-
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
fix(vscode): ts server restart on file change #1424
Conversation
🤖 Pull request artifacts
|
This comment was marked as outdated.
This comment was marked as outdated.
Neat! :D Screen.Recording.2023-05-26.at.11.48.56.mov |
// macOS watcher to be removed in future releases | ||
const rootPath = workspace.workspaceFolders?.[0].uri.path | ||
if (os.platform() === 'darwin' && rootPath !== 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.
Do we know that it generally, really only works on MacOS right now? And not that us while testing just were unlucky on Linux and Windows to have it not work?
(The cost of keeping it running on Linux and Windows seems minimal, if we are not sure if for some users it still does what it should and triggers reloads for old CLI versions)
}, | ||
}) | ||
console.log(`Watching ${rootPath} for changes (old watcher).`) | ||
} else { |
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.
What is the benefit of doing either/or, instead of just both?
Even on MacOS in theory the first old variant could fail - and then the new one could catch it?
console.debug('File Watcher is disabled.') | ||
} | ||
} else { | ||
console.debug('File Watcher was skipped, rootPath is falsy') |
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 special case is not needed any more today?
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.
With the new changes, it does not matter if the rootPath is falsy, the watcher will still work.
// when the file watcher settings change, we need to ensure they are applied | ||
workspace.onDidChangeConfiguration((event) => { | ||
if (event.affectsConfiguration('prisma.fileWatcher')) { | ||
setGenerateWatcher(!!workspace.getConfiguration('prisma').get('fileWatcher')) | ||
} | ||
}), |
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 now here in context.subscriptions.push
vs outside where this was handled before?
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's two ways of writing the same thing.
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 good, we can improve some things later probably, but it's already very valuable for many users 🙌🏼
This PR works with prisma/prisma#19457, we now send a simpler signal for when we need to reload types via our extension, instead of watching many possible paths.
closes prisma/prisma#13946