Skip to content
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

Kiosk: now using the config run url if it exists #10048

Merged
merged 2 commits into from
Jul 25, 2024

Conversation

srietkerk
Copy link
Contributor

@srietkerk srietkerk commented Jul 23, 2024

This change to kiosk is to make sure that we are using the correct run url in our different environments. For example, in stable, we should be using stable in order to test the changes that we're trying to ship or change. We can make sure that's the case by using the webConfig's run url. However, when testing locally, webConfig was null. While I am already specializing local host, I also wanted to keep that case in mind for the live/built scenarios, so I'm keeping the hard-coded "PlayUrlRoot" as a fall back just in case webConfig is null.

Edit: Here's an upload target (this upload target includes some console logs for testing but those console logs are now removed): https://arcade.makecode.com/app/b864d38cd277079aa0d82d37340a2a5c08132a96-51ba117ade--kiosk. You can see in those logs and the network request for when you play a game that the run url being used is according to the environment, which, in this case is the upload target url.

@srietkerk srietkerk requested a review from a team July 23, 2024 23:23
Copy link
Member

@kimprice kimprice left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -16,6 +16,12 @@ export default function PlayingGame() {
return getLaunchedGame();
}, [kiosk.launchedGameId]);

const makeRunUrl = () => {
return pxt.webConfig?.runUrl
? "https://arcade.makecode.com" + pxt.webConfig.runUrl
Copy link
Member

Choose a reason for hiding this comment

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

this will make it so staging still points at prod sim, which isn't ideal -- if leaving it as just a relative url (that is, just using .runUrl with no host domain) works that'd be great, otherwise window.location.origin instead of https://arcade.makecode.com would be good as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I've been testing it with just using pxt.webConfig.runUrl, and it looks like it works. I'm not sure I understand why, though. pxt.Webconfig.runUrl only has the end path of the url, not the domain. Does the browser just resolve and say "just use the same root/domain if one isn't explicitly supplied"?

Copy link
Member

@jwunderl jwunderl Jul 25, 2024

Choose a reason for hiding this comment

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

(discussed in standup but basically answer is 'yes' / paths get treated as relative)

@srietkerk srietkerk merged commit f5184c5 into master Jul 25, 2024
6 checks passed
@srietkerk srietkerk deleted the srietkerk/kiosk-use-configurl branch July 25, 2024 21:23
srietkerk added a commit that referenced this pull request Jul 25, 2024
* now using the config run url if it exists

* use nullish coalescing and relative url paths
srietkerk added a commit that referenced this pull request Jul 26, 2024
* now using the config run url if it exists

* use nullish coalescing and relative url paths
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants