Skip to content

Add and example to consume the WebRtcTCPRelayDetectedEvent #43

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

david-macpherson
Copy link
Contributor

Relevant components:

  • Scalable Pixel Streaming Frontend library
  • Examples
  • Docs

Problem statement:

What problem does this PR address?

Solution

How does this PR solve the problem?

Documentation

If you added a new feature or changed an existing feature please add some documentation here.

Or better yet, link to the relevant /Docs update in your PR.

Test Plan and Compatibility

What steps have you taken to ensure this PR maintains compataibility with the existing functionality?

Or better yet, link to the unit tests that accompany this PR.

@david-macpherson david-macpherson added the enhancement New feature or request label Feb 20, 2024
@david-macpherson david-macpherson self-assigned this Feb 20, 2024
@david-macpherson david-macpherson marked this pull request as ready for review February 22, 2024 23:20
Copy link
Contributor

@dan-tw dan-tw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, left some feedback

},
output: {
filename: '[name].js',
library: 'spstypescriptexample',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Different output name for the library here perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the library name

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this is meant as an 'example' -- can we get some detailed commenting on this file and any others telling us what each thing is and how it is used/meant to be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some more commenting

@dan-tw
Copy link
Contributor

dan-tw commented Feb 26, 2024

Just a thought about this PR. Perhaps we could make use of the environment variables on the typescript example to build it with or without the WebRtcTcpDetectEvent? This would save duplicated the entire example and creating another example that needs to be managed.

Interested to hear options

@@ -61,7 +61,7 @@ export class SPSApplication extends Application {
this.stream.setSignallingUrlBuilder(() => {

// if we have overriden the signalling server URL with a .env file use it here
if (WEBSOCKET_URL) {
if (WEBSOCKET_URL != "") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail in prod where WEBSOCKET_URL is being set as undefined:

plugins: [
    new webpack.DefinePlugin({
      WEBSOCKET_URL: undefined
    }),
  ]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this has been moved to #44 PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants