-
Notifications
You must be signed in to change notification settings - Fork 580
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
Refactor all of our various cra build steps #9907
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.
Huge improvement! I think only the webappNames
issue in server.ts
is likely to cause actual problems. Other comments are just potential opportunities for further simplification, though they may not be worth the trouble. This is a big help either way!
if (pathname == "/--multiplayer") { | ||
sendFile(path.join(publicDir, 'multiplayer.html')); | ||
return | ||
for (const subapp of SUB_WEBAPPS) { |
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.
GitHub won't let me make comments higher up in the file, but up at line 28, there's a webappNames
array we'll need to update for apps that have localServe set to false. Slight quirk in that teachertool's is not actually teachertool, it's "eval". May need add an extra field for publicName or endpointName or something.
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.
ah, good catch. lemme see what i can do here...
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.
fixed!
.pipe(gulp.dest("webapp/public"))); | ||
|
||
const teacherTool = gulp.series(cleanTeacherTool, buildTeacherTool, gulp.series(copyTeacherToolCss, copyTeacherToolJs, copyTeacherToolHtml)); | ||
const teacherTool = createWebappTasks("teachertool"); |
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.
Down below here, we have the "webapp build wrapper" section which calls into the update*Strings functions. That could maybe be made slightly simpler with a unified updateWebappStrings function so we don't have to create a new one for each app, but not a huge deal.
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.
done!
"built/web/teachertool/css/*.css", | ||
"built/web/teachertool/js/*.js", | ||
"built/web/*/css/*.css", | ||
"built/web/*/js/*.js", |
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.
It'd be nice if we could update the npm-prepare.js script (gets called as a part of prepare at the bottom of this file) to also reference subwebapp.ts but I'm not sure if that's possible...
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.
done! I moved this config into a json file that gets used in npm-prepare.js, in the cli, and in the gulpfile
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.
Nice 😎
Maybe not very helpful at this point, but FWIW, I saved a branch which just had the bare-bones updates I needed to make a new CRA. Figured I'd mention it, in case it's a useful reference: master...thsparks/new_react_app#diff-d7ffbddb3a36042f0033638e9e21f556c8873232b040c3f6579488d74adec3daR6 |
I will soon be adding another CRA for the tutorial tool, so I figured it was about time to refactor all of this build code to make it easier to add new apps and generally unify things.
@thsparks since you did this most recently, did I miss anything here?