[Tooling] Update code to DRY the Windows build PS1

[iangmaia]

Internal reference: AINFRA-532

Consolidates build-for-windows-dev.ps1 and build-for-windows-release.ps1 into a single script.

The new build-for-windows.ps1 script can be called with npm run make:windows-release or npm run make:windows-dev.

[mokagio]

I wonder if there's a difference between exit 1 and Exit 1

[iangmaia]

This PS linter rule says:

PowerShell is case insensitive wherever possible, so the casing of cmdlet names, parameters, keywords and operators does not matter. This rule nonetheless ensures consistent casing for clarity and readability.

I'll update it for consistency, thanks for bringing it up!

[mokagio]

One of our other apps that is not open source uses an env var to drive the behavior:

if (-not $env:NODE_ENV) {
    Write-Error "NODE_ENV must be set to either 'production' or 'development'"
    Exit 1
}

if ($env:NODE_ENV -notin @('production', 'development')) {
    Write-Error "NODE_ENV must be either 'production' or 'development', got: $env:NODE_ENV"
    Exit 1
}

Have you considered that option. If so, why did you chose this?

The call parameter is cool in that it would allow us to run release vs dev builds regardless of what's in the environment. But then... is there a use case where we would want to ignore whichever setting is in the environment?

I don't have a favorite implementation, just curious and thinking out loud.

[iangmaia]

Have you considered that option. If so, why did you chose this?

I did consider, but the case of running the script as a function with regular, clearly defined parameters seemed to make the code more obvious specially on the caller side.
It's not a big difference after all, but when reading the other code the first time it took me some time to realize that NODE_ENV was being set in the environment and used as input by the script.

is there a use case where we would want to ignore whichever setting is in the environment?

Is NODE_ENV some sort of standard env var also used by anything other than our own script or something we came up with? If it's used by anything else, then perhaps it's best to just use the env var in the entire call stack 🤔

[mokagio]

the case of running the script as a function with regular, clearly defined parameters seemed to make the code more obvious specially on the caller side

Agreed. Env vars are not as obvious as call parameters.

[mokagio]

Is NODE_ENV some sort of standard env var also used by anything other than our own script or something we came up with?

My understanding, validated by ChatGPT for what is worth, is that it's not a standard but it is a very common convention. ChatGPT tells me it is used in React and Webpack, which are widely popular frameworks.

Git-grepping for it brings up a few results, but I think it's telling the team decided to use a different env var, IS_DEV_BUILD, to drive whether to make a dev or release build.

I think we're good to stick to the implementation you propose here 👍

[mokagio]

TIL.

I wonder if there's a way to wrap this in some code that fails with a helpful message on non-Windows environments. Then again, I don't see devs making builds, on Windows or non-Windows machines, as a use case we ought to optimize for. All builds should run through CI.

[iangmaia]

I wonder if there's a way to wrap this in some code that fails with a helpful message on non-Windows environments

On a Mac it will complain that powershell isn't available (though when you install it with brew, the command is actually named pwsh). I think it is possible but tricky to write something that would work in all environments, and probably not worth it.

[mokagio]

I think it is possible but tricky to write something that would work in all environments, and probably not worth it.

👍

[mokagio]

Looking good so far @iangmaia ! Thank you for tackling this.

[mokagio]

Thanks @iangmaia

I didn't mentioned it yesterday but I hacked #1409 to test the release builds in CI. It was green yesterday and a new build with the latest code changes was also green.

Good to go.

[mokagio]

the case of running the script as a function with regular, clearly defined parameters seemed to make the code more obvious specially on the caller side

Agreed. Env vars are not as obvious as call parameters.

[mokagio]

I think it is possible but tricky to write something that would work in all environments, and probably not worth it.

👍

[mokagio]

Is NODE_ENV some sort of standard env var also used by anything other than our own script or something we came up with?

My understanding, validated by ChatGPT for what is worth, is that it's not a standard but it is a very common convention. ChatGPT tells me it is used in React and Webpack, which are widely popular frameworks.

Git-grepping for it brings up a few results, but I think it's telling the team decided to use a different env var, IS_DEV_BUILD, to drive whether to make a dev or release build.

I think we're good to stick to the implementation you propose here 👍