Add node as npm script runner#236967
Add node as npm script runner#236967connor4312 merged 10 commits intomicrosoft:mainfrom xymopen:main
Conversation
connor4312
left a comment
connor4312
left a comment
There was a problem hiding this comment.
Looking good overall, some comments
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
| 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
extensions/npm/src/tasks.ts
Outdated
| 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
|
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 |
…sk` from `createTask`
Upstream reviewer is ok with current state
|
@connor4312 It's been a while since your last comment. Is there any more adjustment I need to make for this PR? |
This reverts commit 6a5991c.
|
Reverting this #240413 since I couldn't run npm tasks in one of my private repos. I see this thrown repeatedly |
|
Sorry to hear that. Could you please isolate a minimal reproduction so I can look into it? |
|
@roblourens I found the potential cause and open another PR #240527. Please check and let me know if the bug persists. Thanks in advanced. |
3 participants
Fixes #234468
npm.scriptRunnersetting for running scriptsnodeas a script runner