factor common export code into export.js #27772
Conversation
apps/src/export.js
Outdated
| @@ -0,0 +1,138 @@ | |||
| import {SnackSession} from '@code-dot-org/snack-sdk'; | |||
There was a problem hiding this comment.
I wonder if this file should live in apps/src/lib/util instead of the source-root. Just thinking for someone trying to understand our code, this isn't one of the first things we'd want them to read.
There was a problem hiding this comment.
yes, I debated that as well. we have src/, src/util/, src/lib/util/ as potential candidates. I tried to match the convention being used (common code seemed to be at root, re-usable "utilities" seem to be in util) - but I can definitely see the argument against using the root of src
There was a problem hiding this comment.
I still try to stick to the organization proposed in https://github.com/code-dot-org/code-dot-org/blob/8990f16a29d8a1cf9dba0ffb220bd424e8542810/apps/docs/refactor.md although I'm not sure that's a shared goal anymore.
There was a problem hiding this comment.
after merging with staging, I've moved the code into a new file recently created by @davidsbailey called src/util/exporter.js
apps/src/export.js
Outdated
| }, data); | ||
| } | ||
|
|
||
| export function getEnvironmentPrefix() { |
There was a problem hiding this comment.
Not introduced in this PR, but wanted to call out that this big switch statement feels like something that should be a locals/globals.yml configuration setting and passed down to the client, rather than hard-coded in the client code here.
|
Updated after merging |
|
Sorry for missing this, Chris! Thanks for catching. |
|
Thank you for making this change!! +1 on the unit testing comment. |
src/gamelab/Exporter.jsandsrc/applab/Exporter.jsare similar or identical. I've taken the pieces that are essentially identical and moved them intosrc/export.jsto simplify maintenance - and provide a location for further consolidation.