Arc 412 extract and publish npm package #1575

Skyzreal · 2025-11-14T14:04:28Z

Changed @devolutions/multi-video-player and @devolutions/shadow-player from
private to public
Added peer dependencies: @devolutions/shadow-player, video.js
Gateway API client for fetching recording data
build.ps1 for both packages
shadow-player: Added disconnect for proper cleanup

…package' into ARC-412-Extract-and-publish-NPM-package

github-actions · 2025-11-14T14:04:50Z

Let maintainers know that an action is required on their side

Add the label release-required Please cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module) when you request a maintainer to cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module)
Add the label release-blocker Follow-up is required before cutting a new release if a follow-up is required before cutting a new release
Add the label publish-required Please publish libraries (`Devolutions.Gateway.Utils`, OpenAPI clients, etc) when you request a maintainer to publish libraries (Devolutions.Gateway.Utils, OpenAPI clients, etc.)
Add the label publish-blocker Follow-up is required before publishing libraries if a follow-up is required before publishing libraries

github-actions · 2025-11-14T14:43:44Z

Maintainer action requested

Author requested libraries to be published.

cc @CBenoit

refactor(recording): publishing npm package fix(recording-player): use published shadow-player exports instead of internal paths

CBenoit · 2025-11-17T10:04:16Z

@Skyzreal Are you looking for initial feedback on your draft PR?

Skyzreal · 2025-11-17T12:08:06Z

@CBenoit Yes, exactly, I'm taking some time this morning to review the changes and would really appreciate any initial feedback, especially since it’s my first time commiting in the devolutions-gateway repo

CBenoit · 2025-11-19T13:23:28Z

webapp/packages/shadow-player/package.json

-  "private": true,
-  "version": "0.0.0",
+  "private": false,
+  "version": "1.0.0",


suggestion: Let’s start with 0.1.0 (pre v1)

Suggested change

"version": "1.0.0",

"version": "0.1.0",

CBenoit · 2025-11-19T13:23:55Z

webapp/packages/shadow-player/package.json

-  "main": "./dist/shadow-player.js",
-  "module": "./dist/shadow-player.js",
+  "main": "./dist/webm-stream-player.js",
+  "module": "./dist/webm-stream-player.js",


question: Is it intended? I see this is the package.json from shadow-player 🤔

suggestion: Use shadow-player; or better: many packages are using a generic index name for the entry points.

Thanks for noticing! I cleaned it up so the package now uses the standard index

Copilot

Pull Request Overview

This PR prepares two packages (@devolutions/shadow-player and @devolutions/multi-video-player) for publication to npm by changing them from private to public packages. The changes include adding proper peer dependencies, implementing a Gateway API client for fetching recording data, and adding cleanup functionality to the shadow player.

Key changes:

Published shadow-player (v1.0.0) and multi-video-player (v1.0.0) as public npm packages
Implemented GatewayRecordingApi client for Gateway recording operations
Added disconnect() method to shadow-player for proper resource cleanup

Reviewed Changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
webapp/pnpm-lock.yaml	Added shadow-player as a workspace dependency for multi-video-player
webapp/packages/shadow-player/src/streamer.ts	Added disconnect method, session-not-found error type, and improved WebSocket lifecycle management
webapp/packages/shadow-player/package.json	Changed to public package v1.0.0 with updated entry points
webapp/packages/shadow-player/build.ps1	Added PowerShell build script for package distribution
webapp/packages/multi-video-player/vite.config.ts	Externalized video.js and shadow-player dependencies
webapp/packages/multi-video-player/src/live-streamer.ts	New class for managing live recording streams with shadow-player
webapp/packages/multi-video-player/src/index.ts	Exported gateway-api and live-streamer modules
webapp/packages/multi-video-player/src/gateway-api.ts	New Gateway API client for recording operations
webapp/packages/multi-video-player/package.json	Changed to public package v1.0.0 with peer dependencies
webapp/packages/multi-video-player/build.ps1	Added PowerShell build script for package distribution
webapp/apps/recording-player/src/streamers/webm.ts	Updated imports to use published package paths
.github/workflows/publish-libraries.yml	Added multi-video-player and shadow-player to npm publishing workflow

Files not reviewed (1)

webapp/pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-19T13:27:37Z

webapp/packages/shadow-player/src/streamer.ts

  }
+
+  public disconnect(): void {
+    this.isDisconnecting = true;


The isDisconnecting flag is set to true at the start of disconnect() but is never reset to false. If the shadow player component is reused after calling disconnect(), subsequent operations might behave incorrectly. Consider resetting this flag after cleanup is complete, or document that the component should not be reused after disconnect.

Copilot · 2025-11-19T13:27:38Z

webapp/packages/shadow-player/package.json

  "types": "./dist/shadow-player.d.ts",
+  "exports": {
+    ".": {
+      "import": "./dist/webm-stream-player.js",
+      "types": "./dist/shadow-player.d.ts"


The main and module fields point to webm-stream-player.js but the exports field also points to webm-stream-player.js, while the types field points to shadow-player.d.ts. This inconsistency in naming could cause issues. The main bundle filename appears to be webm-stream-player.js but the type definitions use a different basename (shadow-player). Consider ensuring these filenames are consistent or verify that the build process generates both correctly.

Suggested change

"types": "./dist/shadow-player.d.ts",

"exports": {

".": {

"import": "./dist/webm-stream-player.js",

"types": "./dist/shadow-player.d.ts"

"types": "./dist/webm-stream-player.d.ts",

"exports": {

".": {

"import": "./dist/webm-stream-player.js",

"types": "./dist/webm-stream-player.d.ts"

Copilot · 2025-11-19T13:27:38Z

webapp/packages/multi-video-player/package.json

+    "vite-plugin-bundle-css": "^0.1.1"
+  },
+  "peerDependencies": {
+    "@devolutions/shadow-player": "workspace:*",


The peer dependency for @devolutions/shadow-player is set to workspace:*, but this is not a valid version specifier for published packages. When this package is published to npm, workspace:* won't resolve correctly for consumers outside the workspace. This should be changed to a semantic version range like ^1.0.0 to match the published version of shadow-player.

Suggested change

"@devolutions/shadow-player": "workspace:*",

"@devolutions/shadow-player": "^1.0.0",

Probably needs to be addressed, but I’m not sure how we are supposed to deal with that with npm.

suggestion: Refer to my suggestion about the "two package.json files" approach.

Copilot · 2025-11-19T13:27:38Z

webapp/packages/shadow-player/src/streamer.ts

+      if (reactiveSourceBuffer && mediaSource.readyState === 'open') {
+        try {
+          mediaSource.endOfStream();
+        } catch (error) {


The error in the catch block is being silently swallowed without any logging or handling. Consider at least logging the error for debugging purposes, or add a comment explaining why it's safe to ignore this error.

Suggested change

} catch (error) {

} catch (error) {

console.error('endOfStream error:', error);

Copilot · 2025-11-19T13:27:39Z

webapp/packages/shadow-player/src/streamer.ts

+      } catch (error) {
+      }


The error in the catch block is being silently swallowed without any logging or handling. Consider at least logging the error for debugging purposes, or add a comment explaining why it's safe to ignore this error.

Copilot · 2025-11-19T13:27:39Z

webapp/packages/multi-video-player/src/gateway-api.ts

+      }
+
+      const sessions = await response.json();
+      const sessionExists = sessions.some((session: any) => session.id === this.sessionId);


Using any type defeats the purpose of TypeScript's type safety. Consider defining a proper interface for the session object, e.g., interface Session { id: string; ... } and typing the sessions array appropriately.

Copilot · 2025-11-19T13:27:40Z

webapp/packages/shadow-player/build.ps1

@@ -0,0 +1,19 @@
+#!/bin/env pwsh


The shebang line is incorrect. It should be #!/usr/bin/env pwsh instead of #!/bin/env pwsh to properly locate the pwsh interpreter using the env command.

Suggested change

#!/bin/env pwsh

#!/usr/bin/env pwsh

Copilot · 2025-11-19T13:27:40Z

webapp/packages/multi-video-player/build.ps1

@@ -0,0 +1,19 @@
+#!/bin/env pwsh


The shebang line is incorrect. It should be #!/usr/bin/env pwsh instead of #!/bin/env pwsh to properly locate the pwsh interpreter using the env command.

Suggested change

#!/bin/env pwsh

#!/usr/bin/env pwsh

Copilot · 2025-11-19T13:27:40Z

webapp/packages/shadow-player/src/streamer.ts

+      } catch (error) {
+      }


The error in the catch block is being silently swallowed without any logging or handling. Consider at least logging the error for debugging purposes, or add a comment explaining why it's safe to ignore this error.

Copilot · 2025-11-19T13:27:41Z

webapp/packages/multi-video-player/src/gateway-api.ts

+
+      return sessionExists;
+    } catch (error) {
+


The error in the catch block is being silently swallowed without any logging or handling. Consider at least logging the error for debugging purposes so issues can be diagnosed if the session check fails unexpectedly.

Suggested change

console.error('Error checking session activity:', error);

CBenoit · 2025-11-19T13:59:28Z

Please follow this Git workflow: https://github.com/Devolutions/devolutions-gateway/blob/4304148f0979809d3941543d2083101abc293840/AGENTS.md#git-workflow
There is no library scope yet for these, but you can use npm for now.

CBenoit · 2025-11-19T14:13:06Z

webapp/packages/multi-video-player/package.json

+  "peerDependencies": {
+    "@devolutions/shadow-player": "workspace:*",
+    "video.js": "^8.0.0"
  },


question: Why are they "peer dependencies"? I’m not super familiar with that.

CBenoit · 2025-11-19T14:15:41Z

webapp/packages/multi-video-player/package.json

-  "private": true,
-  "version": "0.0.0",
+  "private": false,
+  "version": "1.0.0",


suggestion: Let’s start with 0.1.0

CBenoit

I’m not an expert on npm packages, but I’ve done some digging, and I have some questions. Let’s check that together.

I’m not able to build the npm packages locally by running the scripts, did you check that?

For ts-angular-client, the dist folder has a package.json:

The package.json in dist is the one containing the "exports" key and other publishing related keys, not the top-level one.

I think the "two package.json approach" is the preferred pattern for TypeScript packages. It ensures we only publish a clean JS/d.ts only package using the resulting artifacts found in the dist/ folder. Also avoid import {…} from 'package/dist' in favor of import {…} from 'package', avoid unnecessary dev metadata, etc.

suggestion: Keep the root package.json files private ("private": true). Keep the types, module, main, exports and other runtime dependencies in the generated dist/package.json file.

You can either use a build script, or the vite-plugin-static-copy vite plugin. At least for IronRDP and IronVNC we use the vite-plugin-static-copy approach, you can refer to it: https://github.com/Devolutions/IronRDP/tree/master/web-client/iron-remote-desktop

note: Make sure to keep the publish-related metadata in the dist/package.json file only. In my comments I mention the description key. This must be in dist/package.json.

CBenoit · 2025-11-19T19:23:48Z

webapp/packages/multi-video-player/package.json

  "dependencies": {
    "vite-plugin-dts": "^4.5.3",
    "vite-plugin-static-copy": "^2.3.0"
  },


issue?: Probably needs to be in devDependencies? Double check if these are development tools.

CBenoit · 2025-11-19T19:24:44Z

webapp/packages/shadow-player/package.json

    "ts-node": "^10.9.2",
    "vite-plugin-dts": "^4.3.0"


issue: I’m pretty sure those are development tools.

CBenoit · 2025-11-19T19:25:23Z

webapp/packages/shadow-player/package.json

issue: Missing description field.

CBenoit · 2025-11-19T20:16:35Z

webapp/packages/multi-video-player/package.json

+    "vite-plugin-bundle-css": "^0.1.1"
+  },
+  "peerDependencies": {
+    "@devolutions/shadow-player": "workspace:*",


suggestion: Refer to my suggestion about the "two package.json files" approach.

CBenoit · 2025-11-19T20:24:39Z

Changed @devolutions/multi-video-player and @devolutions/shadow-player from
private to public

Added peer dependencies: @devolutions/shadow-player, video.js

Mentioned that in one of my comments, but I would like some explanation on the rationale (mostly because I’m not familiar with that).

* Gateway API client for fetching recording data

[…]

* shadow-player: Added disconnect for proper cleanup

Note that it’s preferable to open multiple pull requests: one per change.

For instance:

One PR adding the Gateway API client code
One PR adding the proper disconnect logic
One PR for preparing the packages for publishing

It’s much easier to review, and easier to track the changes properly. Overall it’s easier to quickly merge multiple smaller PRs.

Skyzreal · 2025-11-21T17:31:54Z

Thanks for the detailed feedback, Benoit! You're absolutely right, this should have been 3 separate PRs. I’ll make sure to keep future PRs smaller and more focused.

I should've written better description in my PR description, but basically the gateway-api is used to fetch data. It used to be scattered, but my idea is to have a reusable API (I have not touched the previous code so other projects can still use what is there).

For the shadow-player disconnect method, that one is just for proper cleanup. When testing with DVLS, I noticed WebSocket connections and video buffers stayed alive even after closing the dialog, so this makes sure everything is freed correctly.

Regarding dependencies: I initially used peerDependencies because, from what I could see, they can help avoid duplicates and let users manage their versions (especially for something big like video.js). I wasn't sure how RDM handled packages, so I wanted to avoid duplications (having 2 video.js), but after looking into it more, modern package managers (npm 7+) dedupe automatically, and regular dependencies give a simpler install experience, so I made the change to switch to regular dependencies

Right now I’m updating the packages to follow the two-package.json approach, working on that fix now

Skyzreal added 3 commits October 30, 2025 14:50

refactor(recording): publishing npm package

d861a5d

refactor(recording): publishing npm package

00527b7

Merge remote-tracking branch 'origin/ARC-412-Extract-and-publish-NPM-…

4a381ae

…package' into ARC-412-Extract-and-publish-NPM-package

Skyzreal requested review from a team as code owners November 14, 2025 14:04

Skyzreal marked this pull request as draft November 14, 2025 14:43

Skyzreal added the publish-required Please publish libraries (`Devolutions.Gateway.Utils`, OpenAPI clients, etc) label Nov 14, 2025

Skyzreal force-pushed the ARC-412-Extract-and-publish-NPM-package branch 2 times, most recently from 2307372 to d400247 Compare November 14, 2025 16:30

refactor(recording): publishing npm package

14a9bf4

refactor(recording): publishing npm package fix(recording-player): use published shadow-player exports instead of internal paths

Skyzreal force-pushed the ARC-412-Extract-and-publish-NPM-package branch from d400247 to 14a9bf4 Compare November 14, 2025 16:37

Skyzreal marked this pull request as ready for review November 17, 2025 16:09

CBenoit requested a review from Copilot November 19, 2025 13:20

Copilot started reviewing on behalf of CBenoit November 19, 2025 13:21 View session

Copilot finished reviewing on behalf of CBenoit November 19, 2025 13:23

CBenoit reviewed Nov 19, 2025

View reviewed changes

Copilot AI reviewed Nov 19, 2025

View reviewed changes

CBenoit reviewed Nov 19, 2025

View reviewed changes

	"@devolutions/shadow-player": "workspace:*",
	"@devolutions/shadow-player": "^1.0.0",

	} catch (error) {
	} catch (error) {
	console.error('endOfStream error:', error);

Arc 412 extract and publish npm package #1575

Are you sure you want to change the base?

Arc 412 extract and publish npm package #1575

Uh oh!

Conversation

Skyzreal commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 14, 2025

Let maintainers know that an action is required on their side

Uh oh!

github-actions bot commented Nov 14, 2025

Maintainer action requested

Uh oh!

CBenoit commented Nov 17, 2025

Uh oh!

Skyzreal commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CBenoit Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

CBenoit Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

CBenoit commented Nov 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CBenoit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Skyzreal commented Nov 14, 2025 •

edited

Loading

Skyzreal commented Nov 17, 2025 •

edited

Loading

CBenoit Nov 19, 2025 •

edited

Loading

CBenoit Nov 19, 2025 •

edited

Loading

Skyzreal commented Nov 21, 2025 •

edited

Loading