-
Notifications
You must be signed in to change notification settings - Fork 78
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
Improve Memory support for role based creeps and the like #246
base: master
Are you sure you want to change the base?
Conversation
e6f41b1
to
5f52bc5
Compare
I think this change offers a great opportunity to define stronger types for creeps memory which leads to cleaner code. In my opinion, if the examples in the tests used It would be nice to add these examples to the Readme for more visibility because it seems like better practice to define strong types this way compared to the alternative options. Great work! |
I’ve been testing this for a few days now and find this is much better than the previous lists of optional properties. I can’t see the advantage of the Brand<> part over the explicit typing, but then my TS experience is limited to Screeps. |
cee12d4
to
542257b
Compare
No real reason to use |
Just trying to work out the Brand<> part - but your example in the description seems incomplete, the Brand type isnt' defined? |
Ah, sorry. Yes, that is correct. I will add it as a note, but currently I define it as seen here typed-screeps/dist/screeps-tests.ts Line 22 in 896ac3f
Pushed a change and named it |
7f3680a
to
7d8f850
Compare
Problem
It is currently impossible to have memory types that make use of union types with type guards to narrow properties.
Assume we have the following types for our various roles.
There is currently two ways to type this:
The first method has the obvious issue of requiring unnecessary checks for undefined, or unsafe use of the non-null assertion operator, the type also get's rather large, and you may end up reusing a property for the a lot of different purposes with various types.
The second method adds an extra unnecessary level of nesting, but does allow you to safely access memory based on the
role
property thatAttachRole
adds.The solution
This PR adds a solution that allows us to fully control the memory type.
There is a caveat. This solution prevents us from merging
CreepMemory
and the likes from multiple files.In my opinion it shouldn't be done anyway, and isn't necessary - you should just import them and add them to the
*Memory
type above.Other than that, we should have full support of what could be done before.
Try it out
Use either of the below commands, depending on your package manager
This should update your package.json to contain a reference to my branch:
Checklists
src/
files notindex.d.ts
npm run dtslint
to updateindex.d.ts