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

Fix typing of creep.body[n].boost to use mineral boosts #257

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

Conversation

tiennou
Copy link
Contributor

@tiennou tiennou commented Feb 26, 2024

Brief Description

This fixes the typing of const BOOSTS so that the keyof (typeof BOOSTS[T]) that's used for BodyPartDefinition.boost doesn't degrade into string | number (number not being a thing).

It's a bit mingled in with more changes I was trying to get in from Overmind about boost modifiers (the keys to the last level of BOOSTS). Tell me if it's not something we care about, and I think that can be taken out. The other thing I'd wanted to do was to use the actual RESOURCE_* constants for the 2nd key-level, but IIRC that got me into type-trouble.

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
Copy link
Contributor

Jomik commented Feb 26, 2024

To me it still seems wrong to have the record type. Why do we want this?
Also can we add a test case that supports this change? 😊

@@ -724,7 +723,7 @@ declare const BOOSTS: {
damage: 0.3;
};
};
};
} & Record<BodyPartConstant, Record<MineralBoostConstant, Record<BoostModifier, number>>>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} & Record<BodyPartConstant, Record<MineralBoostConstant, Record<BoostModifier, number>>>;
} & Record<BodyPartConstant, Record<MineralBoostConstant, Record<BoostModifier, number>>>;

I think this change ruins some indexing into BOOSTS that people may expect to do.
I would argue that there are better solutions to that - I don't even think this Record type should be there, nor the BoostModifier type.
Normal iterations result in a key of type string, thus you can only use it on objects with an index signature Record<string, T>.

I think it is a bad idea to just add that to all objects though...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what's the fix then. Right now, the code I have fails unless I overlay that over BOOSTS. And to be fair the initial issue is about creep.body[0].boost being wrong. That's just the part I brought from OM since I think it acts as documentation over what the keys actually are.

Also, I'm using my own wrappers over Object.keys/values/entries to make them return the correct key/value/tuple type instead of string, which maybe that's just a me thing.

@tiennou tiennou force-pushed the fix/mineral-boosts branch 2 times, most recently from 8b699b4 to a4ddcaf Compare February 26, 2024 20:18
for (const bodyPart of Object.keys(BOOSTS) as BodyPartConstant[]) {
const boosts = BOOSTS[bodyPart];
for (const mineral of Object.keys(boosts) as MineralBoostConstant[]) {
const upgrades = boosts[mineral];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the fixes, this fails with

Element implicitly has an 'any' type because expression of type 'MineralBoostConstant' can't be used to index type '{ [boost: string]: { [action: string]: number; }; } | { UO: { harvest: 3; }; UHO2: { harvest: 5; }; XUHO2: { harvest: 7; }; LH: { build: 1.5; repair: 1.5; }; LH2O: { build: 1.8; repair: 1.8; }; XLH2O: { build: 2; repair: 2; }; ... 5 more ...; XGH2O: { ...; }; } | ... 5 more ... | { ...; }'.
  Property 'UH' does not exist on type '{ [boost: string]: { [action: string]: number; }; } | { UO: { harvest: 3; }; UHO2: { harvest: 5; }; XUHO2: { harvest: 7; }; LH: { build: 1.5; repair: 1.5; }; LH2O: { build: 1.8; repair: 1.8; }; XLH2O: { build: 2; repair: 2; }; ... 5 more ...; XGH2O: { ...; }; } | ... 5 more ... | { ...; }'.ts(7053)


const key = BoostTypeToBoostArray[type];
const partBoost = BOOSTS[bodyPart];
if (!partBoost || !(boost in partBoost) || !partBoost[boost] || !(key in partBoost[boost])) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the partBoost[boost] checks error with

Element implicitly has an 'any' type because expression of type 'string | number' can't be used to index type '{ [boost: string]: { [action: string]: number; }; } | { UO: { harvest: 3; }; UHO2: { harvest: 5; }; XUHO2: { harvest: 7; }; LH: { build: 1.5; repair: 1.5; }; LH2O: { build: 1.8; repair: 1.8; }; XLH2O: { build: 2; repair: 2; }; ... 5 more ...; XGH2O: { ...; }; } | ... 5 more ... | { ...; }'.
  No index signature with a parameter of type 'string' was found on type '{ [boost: string]: { [action: string]: number; }; } | { UO: { harvest: 3; }; UHO2: { harvest: 5; }; XUHO2: { harvest: 7; }; LH: { build: 1.5; repair: 1.5; }; LH2O: { build: 1.8; repair: 1.8; }; XLH2O: { build: 2; repair: 2; }; ... 5 more ...; XGH2O: { ...; }; } | ... 5 more ... | { ...; }'.ts(7053)

@@ -724,7 +723,7 @@ declare const BOOSTS: {
damage: 0.3;
};
};
};
} & Record<BodyPartConstant, Record<MineralBoostConstant, Record<BoostModifier, number>>>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what's the fix then. Right now, the code I have fails unless I overlay that over BOOSTS. And to be fair the initial issue is about creep.body[0].boost being wrong. That's just the part I brought from OM since I think it acts as documentation over what the keys actually are.

Also, I'm using my own wrappers over Object.keys/values/entries to make them return the correct key/value/tuple type instead of string, which maybe that's just a me thing.

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.

2 participants