Move all of the test webviews and commands behind an env var

[lyonsil]

Changes:

• Move a lot of test code behind an environment variable called DEV_NOISY. Devs can set that environment variable to "true" to see the same test webviews and commands that existed previously.
• Remove the commands named "test.addThree" and "test.squareAndConcat" that were registered inside of the renderer within the command service. The command service was still organized in the old way where flags like isRenderer and isClient are sprinkled into the code, so although the service is in shared, it actually has non-shared portions. Removing these commands eliminates all of that non-shared code, so the command service is all truly now "shared".
• Remove the commands named "test.echoRenderer" and "'helloSomeone.echoSomeoneRenderer" which just wrapped calls to "test.addThree" internally.
• Fix several small bugs with some built-in extensions.

Note that you will want to clear local storage in the renderer when switching DEV_NOISY back and forth to see all webviews appropriately.

---

This change is 

[irahopkinson]

Wow, thanks for all this work

Reviewed 29 of 29 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lyonsil)

---

src/main/main.ts line 128 at r1 (raw file):

    mainWindow.loadURL(
      `${resolveHtmlPath('index.html')}${globalThis.isNoisyDevModeEnabled ? DEV_MODE_RENDERER_INDICATOR : ''}`,

BTW I thought we could just use globalThis even in the render, so I'm wondering why we needed to pass it this way?

Code quote:

DEV_MODE_RENDERER_INDICATOR

---

src/renderer/services/papi-frontend.service.ts line 60 at r1 (raw file):

   * Miscellaneous utility functions and classes
   */
  utils: papiUtil,

Err... aren't we just importing this wherever we need it rather than accessing it from PAPI since its essentially an external package?

[irahopkinson]

FYI I'm testing locally now but thought I'd give the feedback first.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lyonsil)

[irahopkinson]

Tested fine locally .

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lyonsil)

[lyonsil]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson)

---

src/main/main.ts line 128 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW I thought we could just use globalThis even in the render, so I'm wondering why we needed to pass it this way?

globalThis does work in the renderer, but the indicator of being in dev mode can't be set the same way in the browser and Node.js, and globalThis values from main don't automatically flow into all of the processes it creates. In the main process and extension host, we can use process.env to inspect environment variables, but that is only available in Node.js. In the renderer, there is no built-in way to check environment variables. So the main process needs to tell the renderer at start up whether it is in dev mode. Communicating from main to the renderer can be done in a few ways, but since it's just a one time thing done at process start up, I followed advice that I found on StackOverflow. I know I could also use Electron's IPC mechanism, but that is more heavyweight than I needed, and I need something I can run prior to any other code running in index.tsx. The URL option fit that nicely without much complication.

I know there are a lot of files in this PR, but if you look at every change I made to a file named global-this.model.ts, you'll see how globalThis.isNoisyDevModeEnabled is set in each of the TS processes. For C# everything was handled in one file, Program.cs.

---

src/renderer/services/papi-frontend.service.ts line 60 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Err... aren't we just importing this wherever we need it rather than accessing it from PAPI since its essentially an external package?

Yes that can work, too. When papiUtil was removed initially (at the time of creating the separate lib), one of the built-in extensions that use it wasn't changed, so one of the test webviews has been broken for a while. I just put the utils back as they were to fix it, but I can go change the extension to import the lib and use the functions instead.

[irahopkinson]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

src/main/main.ts line 128 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

globalThis does work in the renderer, but the indicator of being in dev mode can't be set the same way in the browser and Node.js, and globalThis values from main don't automatically flow into all of the processes it creates. In the main process and extension host, we can use process.env to inspect environment variables, but that is only available in Node.js. In the renderer, there is no built-in way to check environment variables. So the main process needs to tell the renderer at start up whether it is in dev mode. Communicating from main to the renderer can be done in a few ways, but since it's just a one time thing done at process start up, I followed advice that I found on StackOverflow. I know I could also use Electron's IPC mechanism, but that is more heavyweight than I needed, and I need something I can run prior to any other code running in index.tsx. The URL option fit that nicely without much complication.

I know there are a lot of files in this PR, but if you look at every change I made to a file named global-this.model.ts, you'll see how globalThis.isNoisyDevModeEnabled is set in each of the TS processes. For C# everything was handled in one file, Program.cs.

Yes, I saw that later in the code review, thanks for the explanation, good choice.