fix: exclude suppressImplicitAnyIndexErrors from tsconfig and fix type errors #616
Conversation
1 similar comment
tsconfig.json
Outdated
| // "removeComments": true, /* Do not emit comments to output. */ | ||
| // "noEmit": true, /* Do not emit outputs. */ | ||
| "suppressImplicitAnyIndexErrors": true, | ||
| // "suppressImplicitAnyIndexErrors": true, |
| } | ||
| Object.keys(stringInputs).forEach(key => { | ||
| if (!stringValidator.validate(stringInputs[key])) { | ||
| if (!stringValidator.validate(stringInputs[key as keyof unknown])) { |
There was a problem hiding this comment.
It seems better to define 'feature_key' | 'user_id' | 'variable_key' | 'experiment_key' | 'event_key' as a type, then coerce key to that. I'm not sure what keyof unknown means. I thought keyof was only to be used on object types or types with index signatures.
| if (typeof attributes === 'object' && !Array.isArray(attributes) && attributes !== null) { | ||
| Object.keys(attributes).forEach(function(key) { | ||
| if (typeof attributes[key] === 'undefined') { | ||
| if (typeof attributes[key as keyof unknown] === 'undefined') { |
There was a problem hiding this comment.
Same comment about keyof unknown. If we really want to do this, let's add a comment at least explaining what keyof unknown is doing. Alternatively we could coerce attributes to { [key: string]: unknown }, then we could index it with key. IMO this would be more verbose but clearer.
yavorona
force-pushed
the
pnguen/fix-type-errors
branch
from
28ceb19 to
c0af81f
Compare
mjc1283
left a comment
mjc1283
left a comment
There was a problem hiding this comment.
LGTM. I added a few suggestions. Thanks for fixing this.
| export type InputKey = 'feature_key' | 'user_id' | 'variable_key' | 'experiment_key' | 'event_key' | 'variation_id'; | ||
|
|
||
| export type StringInputs = Partial<Record<InputKey, unknown>>; |
There was a problem hiding this comment.
Are these only used in lib/optimizely.ts? Not sure they need to be in shared_types.
| const errorHandler = (config as { [key: string]: unknown })['errorHandler']; | ||
| const eventDispatcher = (config as { [key: string]: unknown })['eventDispatcher']; | ||
| const logger = (config as { [key: string]: unknown })['logger']; | ||
| if (errorHandler && typeof (errorHandler as { [key: string]: unknown })['handleError'] !== 'function') { |
There was a problem hiding this comment.
Maybe try this to make it less verbose:
config = config as { [key: string]: unknown };There was a problem hiding this comment.
It would work only if I assign it to a new variable.
Since we are using { [key: string]: unknown } to avoid these type errors, I think it would make sense to save this type in shared_types as ObjectWithUnknownProperties. WDYT @mjc1283
There was a problem hiding this comment.
Sounds good. If we need it in several modules, it makes sense to define it in shared_types.
yavorona
force-pushed
the
pnguen/fix-type-errors
branch
from
b3b3917 to
15ec3c5
Compare
Summary
This pr created in response to sdk issue #613
The steps to reproduce and fix the issues were:
"suppressImplicitAnyIndexErrors": truein TSConfigTest plan
Existing FSC and unit tests