fix: do not use dynamic module in post-script for PowerShell

[lewis-yeung]

[Outdated] This has been superseded by #254.

[lewis-yeung]

@possum-enjoyer Hi, Jacky. I got your message. I'm so happy that you want to help with it. I have tested my solution (see this commit in my fork) and it worked just fine with nvs auto on and Oh My Posh (hereinafter OMP) prompt rendering.

However, there is still a flaw if you have a node segment in OMP. E.g., assuming the current used node version is 18.5.0, the first time you change to a project directory in which a .nvmrc file holds the version 18.6.0 (assuming any version has been properly installed), the template {{.Full}} {{.Mismatch}} of the segment will render a wrong value 18.5.0 true, while in any subsequent spawned prompt (the working directory doesn't change), it will be the correct value 18.6.0 false. 😮 This is caused by the invocation order: the original OMP-defined prompt function is the first, and NVS auto-switching code the next.

How about changing the order? That will introduce another problem: we know the init script omp.ps1 has to capture the most recent value of $? in prompt, so if we swap the order, OMP cannot get the correct error code from a $? value after NVS auto-switching done. Since not all NVS users use OMP (they may use Starship or others), it is not that reasonable for NVS to save the last error status in a global variable specifically for OMP.

Quite a tough problem. 😫 I have no idea yet. What do you think?

[possum-enjoyer]

@lewis-yeung thank you so much for trying to solve this issue this is very kind of you. i see your problem. This is a tricky one. I am currently on your no-dynamic-module Branch to test with the import the dynamic module.

This is sure a hard one. One possible solution would be to end the nvs auto on script in either an error or nothing according to the value of $? and logging all errors that came with nvs auto on separatly? I know this is quite hacky but all errors thrown by nvs auto on should be logged anyways and using nvs auto wouldn't be affected if we handle this after this line

Alternatively, maybe fnms solution to this problem could help because neither of those problems are present as far as i can see

Edit: what if we don't change the prompt function but rather the cd alias and use the nvs auto for the new path after we run set-location?

[possum-enjoyer]

I created a wip which might solves the problem. I save $LastExitCode + $? before the nvs auto on script happenend and change the exitcode to the exitcode before or to 1 if $? was false but there was no exit code. Maybe i did oversee something but the small array of tests i did were working

[lewis-yeung]

what if we don't change the prompt function but rather the cd alias and use the nvs auto for the new path after we run set-location?

Like what it does in a .sh post-script? I've considered this approach, however it's not the best solution. A user may have a custom alias/function cd, since NVS auto-switching will append the output of how PATH changes, such output is intrusive to a custom alias/function's original. Moreover, Set-Location may be used directly in some user aliases/functions and these commands also change the CWD, so we make it the gold standard to detect the change of CWD after each command execution.

Alternatively, maybe fnms solution to this problem could help because neither of those problems are present as far as i can see

Hmm... FNM uses a custom alias cd_with_fnm instead of the common cd. For backward compatibility of NVS, as it stands, patching a prompt function may not be perfect, but I don't think it's a bad option.

I save $LastExitCode + $? before the nvs auto on script happenend and change the exitcode to the exitcode before or to 1 if $? was false but there was no exit code. Maybe i did oversee something but the small array of tests i did were working

Of course we can save $? and $LastExitCode and make a tweak in omp.ps1 correspondingly, though it is hacky. On the other hand, just as I mentioned above, if we do so, we have to add some descriptions about it, which make it known to maintainers of any prompt customization tools so they can make tweaks on their projects. This approach is what I wanna continue for the time being.

[lewis-yeung]

@possum-enjoyer I have created a new fix for NVS and with this we can tweak these lines of the prompt function in omp.ps1 for OMP like:

# store if the last command was successful
if ($null -ne $global:NVS_ORIGINAL_LASTEXECUTIONSTATUS) {
    # we have to do this if NVS auto-switching is enabled
    $lastCommandSuccess = $global:NVS_ORIGINAL_LASTEXECUTIONSTATUS
} else {
    $lastCommandSuccess = $?
}

Tested on my end, all worked fine. If you think it is good to go, I'll submit a new PR and ask the author for comments.

[possum-enjoyer]

i mean yeah this would be an easy fix for omp directly i would suggest to add the following line after https://github.com/lewis-yeung/nvs/blob/d973a571b097f52924226cdf1f2999e4dfe8ddb2/lib/auto.js#L126

'$global:NVS_ORIGINAL_LASTEXECUTIONSTATUS = $null' because of the case of omp running without nvs auto on and $global:NVS_ORIGINAL_LASTEXECUTIONSTATUS = $false

[lewis-yeung]

i would suggest to add the following line after https://github.com/lewis-yeung/nvs/blob/d973a571b097f52924226cdf1f2999e4dfe8ddb2/lib/auto.js#L126

'$global:NVS_ORIGINAL_LASTEXECUTIONSTATUS = $null' because of the case of omp running without nvs auto on and $global:NVS_ORIGINAL_LASTEXECUTIONSTATUS = $false

I do have a null check in the code I mentioned above. The $global:NVS_ORIGINAL_LASTEXECUTIONSTATUS won't have a non-null value unless the user sets it manually, which is out of our control.

[possum-enjoyer]

Ahhh nevermind my comment i forgot about line 140 where you removed NVS_ORIGINAL_LASTEXECUTIONSTATUS when you execute nvs auto off

And yeah you are right in that comment i my idea would fiddle too much with backwards compatibility. We can't execute a "pseudo function" right before $global:NVS_ORIGINAL_PROMPT.Invoke() so that $? would have the same value as before, can we?

If not i would say that we your solution is the best one till microsoft decides to release a function to manipulate $? 😁

[lewis-yeung]

@possum-enjoyer I've raised a new PR (#254) for the fix.