Add node as npm script runner #236967
Add node as npm script runner #236967
Conversation
xymopen
force-pushed
the
main
branch
4 times, most recently
from
c5cd0cc to
c1aadd8
Compare
extensions/npm/src/tasks.ts
Outdated
| const scriptRunner = await getScriptRunner(folder, context, showWarning); | ||
| const result = [scriptRunner, scriptRunner === 'node' ? '--run' : 'run']; | ||
| if (workspace.getConfiguration('npm', folder).get<boolean>('runSilent')) { | ||
| result.push('--silent'); |
There was a problem hiding this comment.
--silent will cause an error if the scriptRunner is node, we should ignore that setting in this case
extensions/npm/src/tasks.ts
Outdated
| @@ -104,49 +103,61 @@ function isTestTask(name: string): boolean { | |||
| } | |||
| return false; | |||
| } | |||
| const preScripts: Set<string> = new Set([ | |||
| 'est', /* test typo? */ 'install', 'pack', 'pack', 'publish', 'restart', | |||
There was a problem hiding this comment.
Probably a typo, you can fix it :)
| export async function getScriptRunner(folder: Uri, context?: ExtensionContext, showWarning?: boolean): Promise<string> { | ||
| let scriptRunner = workspace.getConfiguration('npm', folder).get<string>('scriptRunner', 'npm'); |
There was a problem hiding this comment.
I think the default in get here should be auto, and if it's auto then we return getPackageManager(...)
There was a problem hiding this comment.
getPackageManager also defaults to auto and call detectPackageManager. They basically do the same thing, just get different configuration.
extensions/npm/src/tasks.ts
Outdated
| } | ||
| } | ||
| } | ||
| } | ||
| return allTasks; | ||
| } catch (error) { |
There was a problem hiding this comment.
Not sure why it was added initially, but this catch is a no-op, you can remove the try/catch if you want
| const { name, multipleLockFilesDetected: multiplePMDetected } = await findPreferredPM(folder.fsPath); | ||
| const neverShowWarning = 'npm.multiplePMWarning.neverShow'; | ||
| if (showWarning && multiplePMDetected && extensionContext && !extensionContext.globalState.get<boolean>(neverShowWarning)) { | ||
| // todo: add text for npm.scriptRunner? |
There was a problem hiding this comment.
I think we're okay with this text
…sk` from `createTask`
|
Thank you for your review. I've made some adjustment to the code and I believe it is ready for merge. |
|
Also I note that |
Fixes #234468
npm.scriptRunnersetting for running scriptsnodeas a script runner