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
Enable applab data APIs in exported projects #22522
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see this functionality being added! A couple comments to address below before merging.
apps/src/applab/Exporter.js
Outdated
@@ -230,14 +230,23 @@ function extractCSSFromHTML(el) { | |||
const fontAwesomeWOFFRelativeSourcePath = '/assets/fontawesome-webfont.woff2'; | |||
const fontAwesomeWOFFPath = 'applab/fontawesome-webfont.woff2'; | |||
|
|||
function getExportConfigPath(baseHref) { | |||
const curHref = window.location.href; | |||
const curHrefWithoutEdit = curHref.slice(0, curHref.lastIndexOf('/') + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure this works when exporting from a script level like https://studio.code.org/s/csp3/stage/9/puzzle/11 . It's possible that window.dashboard.project.getShareUrl()
will give you what you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, much better using getShareUrl()
!
apps/src/applab/Exporter.js
Outdated
@@ -456,7 +466,10 @@ export default { | |||
const appOptionsJs = getAppOptionsFile(); | |||
const { css, outerHTML } = transformLevelHtml(levelHtml); | |||
const fontAwesomeCSS = exportFontAwesomeCssEjs({fontPath: fontAwesomeWOFFPath}); | |||
const exportBaseHref = `https://studio.code.org/projects/applab/${project.getCurrentId()}/`; | |||
const exportConfigPath = getExportConfigPath(exportBaseHref); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be misunderstanding, but this looks like it won't work in non-production environments (here and below), making it hard to debug and test. Do you currently need to change all of these urls to point to localhost during development? Ideally, the "origin" (https://studio.code.org/ or http://localhost-studio.code.org:3000/) would be passed from the server via appOptions. That work could be done later, but I'd like to be on the same page about whether there is more work to do here.
Also, is this a typo on line 476 below? appOptionsPath: "appOptions.j",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the "Try In Expo" scenario results in apps that are published with links back to our production site. We could change this for testing purposes, but the development server URL is unlikely to work from a mobile device already.
And no, the .j
extension is not a typo. We are forced to bundle our .js
files as .j
files such that the Metro bundler doesn't attempt to parse them and include them in the React Native JavaScript bundle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the hardcoded https://studio.code.org/
URLs. It makes it a little tricky during development for 2 reasons: (1) the lack of a minified applab-api.min.js - but I made that conditional, and (2) the fact that most phones can't access localhost-studio.code.org
since the server is not running on the phone. Dealing with #2 is trickier, but at least phone simulators/emulators can be run on the same IP as the dev server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! It looks like this sets us up better for having end-to-end tests ensuring that (a) previously exported projects do not break, and (b) newly exported projects do not break, which are the two tests I'd want to eventually have for this feature. Also the applab-api logic looks like it will do the right thing in test and circle environments.
As for local development, I must be missing some context -- are we exporting to a format that can only be run from a phone? or is it just the nature of this feature that viewing from a phone is the main use case? If testing on phone is needed, it seems like the developer's options include either running ios simulator locally (easy on mac, hard on linux) or temporarily setting origin
to https://studio.code.org/
while developing. If that's too annoying, I'd be open to putting this back how it was by default, and leaving it up to the eventual UI test writer to figure out how to conditionally turn this origin logic back on.
apps/src/applab/Exporter.js
Outdated
const exportBaseHref = `https://studio.code.org/projects/applab/${project.getCurrentId()}/`; | ||
const exportConfigPath = getExportConfigPath(exportBaseHref); | ||
const exportConfigPath = getExportConfigPath(); | ||
const { origin } = window.location; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦thanks for obtaining this in a way that is much simpler than what I was recommending.
apps/src/applab/Exporter.js
Outdated
@@ -456,7 +466,10 @@ export default { | |||
const appOptionsJs = getAppOptionsFile(); | |||
const { css, outerHTML } = transformLevelHtml(levelHtml); | |||
const fontAwesomeCSS = exportFontAwesomeCssEjs({fontPath: fontAwesomeWOFFPath}); | |||
const exportBaseHref = `https://studio.code.org/projects/applab/${project.getCurrentId()}/`; | |||
const exportConfigPath = getExportConfigPath(exportBaseHref); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! It looks like this sets us up better for having end-to-end tests ensuring that (a) previously exported projects do not break, and (b) newly exported projects do not break, which are the two tests I'd want to eventually have for this feature. Also the applab-api logic looks like it will do the right thing in test and circle environments.
As for local development, I must be missing some context -- are we exporting to a format that can only be run from a phone? or is it just the nature of this feature that viewing from a phone is the main use case? If testing on phone is needed, it seems like the developer's options include either running ios simulator locally (easy on mac, hard on linux) or temporarily setting origin
to https://studio.code.org/
while developing. If that's too annoying, I'd be open to putting this back how it was by default, and leaving it up to the eventual UI test writer to figure out how to conditionally turn this origin logic back on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes LGTM
In order to access the data APIs,
appOptions
needs to contain:firebaseName
,firebaseAuthToken
, andfirebaseChannelIdSuffix
. These are normally supplied by the dashboard when serving up the share page. Since exported projects are not served by the dashboard, we need to solve this in a different way. And since the student's code is going to be executed immediately on page load ascode.js
, we need to populate those values before running some of the existing<script>
tags without making an async API call./projects/applab/<project id>/export_config
?script_call=setExportConfig
query param is specifiedsetExportConfig(<json>)
index.html
to contain:<script>
tag before includingapplab-api.js
that definessetExportConfig()
as a function that stores the config inwindow.EXPORT_OPTIONS
<script>
tag immediately afterwards that calls the new API variant, so thatsetExportConfig()
is called with the actual firebase values and stored inwindow.EXPORT_OPTIONS
applab-api.js
to:window.EXPORT_OPTIONS
withwindow.APP_OPTIONS
before starting applabSTORAGE_COMMANDS
and replaced them with console warningsExample excerpt from an exported
index.html
: