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

Fixing issue #250 #252

Closed
wants to merge 2 commits into from
Closed

Fixing issue #250 #252

wants to merge 2 commits into from

Conversation

petarpetrov88
Copy link

@petarpetrov88 petarpetrov88 commented Feb 20, 2023

A simple solution to Issue #250 and #244

@wbt
Copy link
Contributor

wbt commented Feb 20, 2023

Why is it useful to disallow symbols as keys and allow numbers instead? See #240.

@petarpetrov88
Copy link
Author

Why is it useful to disallow symbols as keys and allow numbers instead? See #240.

Sorry, there is none. I've fixed it.

@wbt
Copy link
Contributor

wbt commented Feb 21, 2023

On a quick read, changing from a colon to in also seems to create a mapped type with all possible strings/symbols - can you describe more about the motivation for that change?

@petarpetrov88
Copy link
Author

On a quick read, changing from a colon to in also seems to create a mapped type with all possible strings/symbols - can you describe more about the motivation for that change?

The previous version doesn't work in TS 3 and above.

@petarpetrov88
Copy link
Author

I've changed it to string | number, because the error i was getting was "An index signature parameter type must be 'string' or 'number'."

@wbt
Copy link
Contributor

wbt commented Feb 21, 2023

I've changed it to string | number, because the error i was getting was "An index signature parameter type must be 'string' or 'number'."

In what version of TS?

@petarpetrov88
Copy link
Author

I've changed it to string | number, because the error i was getting was "An index signature parameter type must be 'string' or 'number'."

In what version of TS?

You can see here that from 3.3.3 till 4.5.5 the old version is throwing an error.

@wbt
Copy link
Contributor

wbt commented Feb 21, 2023

You can see here that from 3.3.3 till 4.5.5 the old version is throwing an error.

I don't see that, from v4.4.4 or v4.5.5.

I also think you're still missing the point of #240.

The question of the colon vs. in is comparing this syntax with this syntax - they serve different purposes, and this MR moves from the first to the second - the question is why?

@petarpetrov88
Copy link
Author

You can see here that from 3.3.3 till 4.5.5 the old version is throwing an error.

I don't see that, from v4.4.4 or v4.5.5.

I also think you're still missing the point of #240.

I don't see how I miss it. I've changed the type to string | symbol so this won't affect it. Regarding the TS version, yes the problem is present till v4.3.5. It's late for me and I'm tired.

@wbt
Copy link
Contributor

wbt commented Feb 22, 2023

The current version of logform requires TS 4.4+, as discussed in #244. If for whatever reason someone can't use a reasonably modern version of TypeScript they can pin Logform to an out-of-date version since they are apparently fine pinning to out-of-date versions of dependencies. There's not really interest in breaking things for people who are running current versions in order to make things better for a few folks who insist on keeping to older technology, especially in the absence of any automated regression testing against a range of older versions. Thanks for your efforts.

@wbt wbt closed this Feb 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
Development

Successfully merging this pull request may close these issues.

2 participants