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

Remove type check, typescript ensures there are numbers #11

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

Remove type check, typescript ensures there are numbers #11

wants to merge 2 commits into from

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