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

[ReadonlyArray] .at() fix undefined issue #1

Merged
merged 7 commits into from Aug 22, 2023

Conversation

DeepDoge
Copy link

This will return undefined regardless if strictNullChecks is true or false.

But this is already the behaviour of native ReadonlyArray .at().

As seen here:

interface Array<T> {
    /**
     * Returns the item located at the specified index.
     * @param index The zero-based index of the desired code unit. A negative index will count back from the last item.
     */
    at(index: number): T | undefined;
}

interface ReadonlyArray<T> {
    /**
     * Returns the item located at the specified index.
     * @param index The zero-based index of the desired code unit. A negative index will count back from the last item.
     */
    at(index: number): T | undefined;
}

Also fixed an issue with index union types, and added tests for it.

Issue was being caused by how Subtract util type works with unions types:

type Foo = Subtract<10, 1 | 2> 
// Foo should give `9 | 8` but gives `9` only

Also handled the cases where index number is a "float" and not an "int" correctly, based on how .at() handles them

Copy link
Owner

@Sheraff Sheraff left a comment

Choose a reason for hiding this comment

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

Nice job, that's clean. Not sure if Matt will want this change, when mine was (slightly) smaller and already too much overhead! But at least yours is complete! And it looks neat too.

Comment on lines 39 to 44
function index() {
return Math.random() > 0.5 ? 0 : 1;
}

const a = arr.at(index());
Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't this be simpler with a forced type like this?

Suggested change
function index() {
return Math.random() > 0.5 ? 0 : 1;
}
const a = arr.at(index());
const index = 1 as 1 | 2;
const a = arr.at(index);

Or is it done like this for "expressivity" of the tests (like saying "sometimes we don't know the type in advance" but with code)?

Comment on lines 50 to 55
function index() {
return Math.random() > 0.5 ? -1 : -2;
}

const a = arr.at(index());
Copy link
Owner

Choose a reason for hiding this comment

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

Same comment as above.

Copy link
Author

Choose a reason for hiding this comment

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

sure

@Sheraff Sheraff merged commit 11f16cc into Sheraff:sheraff/array-at Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants