-
Notifications
You must be signed in to change notification settings - Fork 481
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
[Sprite Lab] Make core library a class #41673
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.
i think the direction you're headed here is great. one thing i'm mulling over is whether or not CoreLibrary
should be a singleton -- what do you think? would we ever want there to be multiple instantiations of this class?
@@ -1,518 +1,563 @@ | |||
var spriteId = 0; |
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.
nit: we'll want to rename this file to CoreLibrary.js
to match since it's a class now
Yeah, I think a singleton would make sense. I'll read up on singleton classes to make sure because I haven't written one in a while, but I don't think we'd ever want multiple |
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.
left a comment about an organizational change i think we should make, but otherwise this refactor looks great. i'm mainly looking at the organization/structure changes, not any implementation changes, so let me know if there's anything i should look at deeper in that regard!
import {commands as actionCommands} from './commands/actionCommands'; | ||
import {commands as behaviorCommands} from './commands/behaviorCommands'; | ||
import {commands as eventCommands} from './commands/eventCommands'; | ||
import {commands as locationCommands} from './commands/locationCommands'; | ||
import {commands as spriteCommands} from './commands/spriteCommands'; | ||
import {commands as worldCommands} from './commands/worldCommands'; | ||
import {commands as validationCommands} from './commands/validationCommands'; |
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.
organizational nit: we should have a ./commands/index.js
file that imports/exports all of these commands files (example here). that makes importing these commands clearer, and we can also alias each export (instead of aliasing them 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.
yayyy, i love seeing refactors like this!
Sorry that this is quite large, but it should be a pure refactor, so hopefully that makes it a little easier to review.
The goal here is to make the spritelab library an instantiable class so that we're not relying on global state that should really be instance variables. This will also make it easier to build Poem Bot as a subclass of CoreLibrary
Links
This PR is a prep work for STAR-1626
Testing story
Pure refactor, so relying on existing test coverage. Also manually spot-checked spritelab and gamelab locally
Deployment strategy
Follow-up work
Privacy
Security
Caching
PR Checklist: