Skip to content

Remove type check, typescript ensures there are numbers#11

Open
rogersm wants to merge 2 commits intoRanvier-TS:masterfrom
rogersm:master
Open

Remove type check, typescript ensures there are numbers#11
rogersm wants to merge 2 commits intoRanvier-TS:masterfrom
rogersm:master

Conversation

@rogersm
Copy link

@rogersm rogersm commented May 16, 2021

Parameters delta and base type are ensured by typescript. No need to have a check to confirm they're numbers.

@seanohue
Copy link
Contributor

IMO we should keep in some runtime checks and this may be a place for us to do so.

Here is why:

  1. The calling code which creates an Attribute, either directly or via AttributeFactory, may be in JavaScript (for the case of someone using a codebase that is partially migrated or that is not using TypeScript itself but uses core-ts to take advantage of upgrades/changes we've made). This case is less important but still worth mentioning.

  2. This code may be called most often when an EffectableEntity (characters, rooms, etc.) is loaded from storage (EffectableEntity#hydrate on line 296 of that file). Depending on how the entity data is stored (e.g. if it is stored as a JSON blob rather than in a database with typed columns or a schema), the data may be invalid, which would be a runtime issue that compiled TypeScript would not catch.

Especially with the loss of some safety & defensive coding as outlined in 2, I would be concerned with merging this unless there is something to be gained by removing these checks (outside of shorter files). It may lead to hard-to-debug issues down the line, though I may just be being paranoid.

Thoughts?

@seanohue seanohue mentioned this pull request May 17, 2021
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