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: noUncheckedIndexedAccess issues #1607

Merged
merged 4 commits into from Nov 5, 2021

Conversation

BarryThePenguin
Copy link
Contributor

@BarryThePenguin BarryThePenguin commented Oct 31, 2021

I fixed the noUncheckedIndexedAccess issues with the non-null assertion operator.

Fixing these issues in a different way would change the behaviour of the code, so I thought it would be helpful to get others input on alternative approaches.

Fixes #1598

@@ -36,7 +36,7 @@ export default class FormatRelative extends Formatter<RelativeTimeFormatOptions>
const { format } = formatOptions;
let unit = formatOptions.unit;
let opts: RelativeTimeFormatOptions = formatOptions;
if (!unit && format && intl.formats.relative && (opts = intl.formats.relative[format])) {
if (!unit && format && intl.formats.relative && (opts = intl.formats.relative[format]!)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An alternative here would be to update line 39 to allow undefined options

let opts: RelativeTimeFormatOptions | undefined = formatOptions;

Which also matches the current behaviour

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah that's actually better

@@ -24,7 +24,7 @@ export default function flatten<T>(src: NestedStructure<T>): Record<string, T> {
const hash = flatten(value);

for (const suffix in hash) {
result[`${key}.${suffix}`] = hash[suffix];
result[`${key}.${suffix}`] = hash[suffix]!;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we could check that hash[suffix] is not undefined, but that would change which keys would be added to result.

We could update result to allow undefined values

const result = new EmptyObject() as Record<string, T | undefined>;

Line 30 has a similar issue

Copy link
Collaborator

Choose a reason for hiding this comment

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

we can check that hash[suffix] is not undefined as well

@@ -15,7 +15,7 @@ function partitionDynamicValuesAndStaticValues(options: Record<string, string |
if (value instanceof Raw) {
staticValues[key] = value.valueOf();
} else {
dynamicValues[key] = value;
dynamicValues[key] = value!;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we could check that value is not undefined, but again, that would change which keys would be added to dynamicValues.

We could update dynamicValues to allow undefined values

const dynamicValues = new EmptyObject() as Record<string, string | undefined>;

Copy link
Collaborator

Choose a reason for hiding this comment

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

let's exclude undefined

@@ -36,7 +36,7 @@ export default class FormatRelative extends Formatter<RelativeTimeFormatOptions>
const { format } = formatOptions;
let unit = formatOptions.unit;
let opts: RelativeTimeFormatOptions = formatOptions;
if (!unit && format && intl.formats.relative && (opts = intl.formats.relative[format])) {
if (!unit && format && intl.formats.relative && (opts = intl.formats.relative[format]!)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah that's actually better

@@ -24,7 +24,7 @@ export default function flatten<T>(src: NestedStructure<T>): Record<string, T> {
const hash = flatten(value);

for (const suffix in hash) {
result[`${key}.${suffix}`] = hash[suffix];
result[`${key}.${suffix}`] = hash[suffix]!;
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can check that hash[suffix] is not undefined as well

@@ -15,7 +15,7 @@ function partitionDynamicValuesAndStaticValues(options: Record<string, string |
if (value instanceof Raw) {
staticValues[key] = value.valueOf();
} else {
dynamicValues[key] = value;
dynamicValues[key] = value!;
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's exclude undefined

@longlho
Copy link
Collaborator

longlho commented Nov 5, 2021

Thanks a lot for your contributions!

@BarryThePenguin BarryThePenguin deleted the typescript branch November 5, 2021 01:46
@ijlee2 ijlee2 added the bug Something isn't working label Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Usage with noUncheckedIndexedAccess: true
4 participants