Skip to content
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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Jomik
Copy link
Contributor

@Jomik Jomik commented Sep 17, 2023

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.

interface MinerMemory {
  sourceId: Id<Source>;
}
interface BuilderMemory {
    targetId?: Id<ConstructionSite>;
}
interface HaulerMemory {
    targetId?: Id<AnyStructure>;
}

There is currently two ways to type this:

// The method using optional properties
interface CreepMemory {
  sourceId?: Id<Source>;
  targetId?: Id<ConstructionSite> | Id<AnyStructure>;
}
// Or using a nested property
type AttachRole<T extends string, O extends object & Partial<Record<"role", never>>> = { role: T } & O;
interface CreepMemory {
  actualMemory:
    | AttachRole<'miner', MinerMemory>
    | AttachRole<'builder', BuilderMemory>
    | AttachRole<'hauler', HaulerMemory>;
}

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 that AttachRole adds.

The solution

This PR adds a solution that allows us to fully control the memory type.

type MyCreepMemory =
  | Brand<'miner', MinerMemory>
  | Brand<'builder', BuilderMemory>
  | Brand<'hauler', HaulerMemory>;

interface Memory {
  creeps: Record<string, MyCreepMemory>;
}

const creep = Game.creeps[name];
if (creep.memory.role === 'hauler') {
  // creep.memory is now of type HaulerMemory
}

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

npm install -D @types/screeps@jomik/typed-screeps#flexible-memory
yarn add -D @types/screeps@jomik/typed-screeps#flexible-memory

This should update your package.json to contain a reference to my branch:

    "@types/screeps": "jomik/typed-screeps#flexible-memory",

Checklists

  • Test passed
  • Coding style (indentation, etc)
  • Edits have been made to src/ files not index.d.ts
  • Run npm run dtslint to update index.d.ts

@Jomik Jomik force-pushed the flexible-memory branch 4 times, most recently from e6f41b1 to 5f52bc5 Compare September 17, 2023 10:42
@Jomik Jomik marked this pull request as draft September 19, 2023 18:38
@Jomik Jomik marked this pull request as ready for review September 24, 2023 10:02
@Jomik Jomik changed the title Flexible memory Improved Memory types Sep 26, 2023
@Jomik Jomik changed the title Improved Memory types Improve Memory support for role based creeps and the like Sep 26, 2023
@admon84
Copy link
Member

admon84 commented Sep 26, 2023

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 role: "harvester"/"builder" instead of type: "foo"/"bar" they would be more clear. Since those are the first two roles introduced in the tutorial, it would create some consistency with the basics.

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!

@Sca1ey
Copy link

Sca1ey commented Sep 28, 2023

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.

README.md Show resolved Hide resolved
dist/screeps-tests.ts Show resolved Hide resolved
@Jomik Jomik force-pushed the flexible-memory branch 2 times, most recently from cee12d4 to 542257b Compare September 28, 2023 18:21
@Jomik
Copy link
Contributor Author

Jomik commented Sep 28, 2023

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.

No real reason to use Brand other than being more DRY I suppose. It prevents you from having a type in your "brand" key, role in the example case.

@Sca1ey
Copy link

Sca1ey commented Sep 29, 2023

Just trying to work out the Brand<> part - but your example in the description seems incomplete, the Brand type isnt' defined?
Should be: type Brand<T,V> = {type:T} & V although maybe Role is a bit more creepy than Brand?

@Jomik
Copy link
Contributor Author

Jomik commented Sep 29, 2023

Just trying to work out the Brand<> part - but your example in the description seems incomplete, the Brand type isnt' defined? Should be: type Brand<T,V> = {type:T} & V although maybe Role is a bit more creepy than Brand?

Ah, sorry. Yes, that is correct. I will add it as a note, but currently I define it as seen here

type Brand<T extends string, O extends object & Partial<Record<"role", never>>> = { role: T } & O;

Pushed a change and named it AttachRole

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants