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

Allow embed timestamp to be Date in typings #587

Merged
merged 4 commits into from Dec 4, 2019
Merged

Allow embed timestamp to be Date in typings #587

merged 4 commits into from Dec 4, 2019

Conversation

oathompsonjones
Copy link
Contributor

@oathompsonjones oathompsonjones commented Dec 2, 2019

Type should be string | Date in order to allow for doing timestamp: new Date()

Type should by `string | Date` in order to allow for doing `timestamp: new Date()`
Skillz4Killz
Skillz4Killz previously approved these changes Dec 2, 2019
`timestamp`s do not work with `string`s at all, the data type must be `Date`
zneix
zneix previously approved these changes Dec 2, 2019
Skillz4Killz
Skillz4Killz previously approved these changes Dec 2, 2019
@Skillz4Killz
Copy link
Contributor

Skillz4Killz commented Dec 2, 2019

I think this PR is fine, but fair warning this could be considered a breaking change. Since it was accepting string before, it would not accept the Date option so most devs probably opted to do something like what I have

new Date(time).toISOString()

Im pretty sure this would break on compile time because you cant set a string to a Date. Will need testing when im able to do so

@Skillz4Killz
Copy link
Contributor

Please, can we consider rewinding this back to the original commit of Date | string? I am certain I am not the only dev who was probably sending strings instead of Date. Here is how many errors I got from this change.

image

I believe the best type would be Date | string

Stop devs gettings loads of errors if they have already been passing strings not dates
Skillz4Killz
Skillz4Killz previously approved these changes Dec 3, 2019
Copy link
Contributor

@Skillz4Killz Skillz4Killz left a comment

Choose a reason for hiding this comment

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

It's perfect. Slight modification to Date | string to have a Strict => Loose typings format would be nicer on the eyes imo.

index.d.ts Outdated Show resolved Hide resolved
@DonovanDMC
Copy link
Contributor

DonovanDMC commented Dec 3, 2019

I have always used new Date().toISOString(), making it take only a Date object would break literally every single one of my projects, thousands of files total (yes, I see that it has been changed back)

Co-Authored-By: Skillz4Killz <23035000+Skillz4Killz@users.noreply.github.com>
@abalabahaha abalabahaha changed the title Changed type for EmbedBase.timestamp Allow embed timestamp to be Date in typings Dec 4, 2019
@abalabahaha abalabahaha merged commit 65dda68 into abalabahaha:dev Dec 4, 2019
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.

None yet

5 participants