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

Vet v6.0.0-alpha.5 types emitted types against v5.5.3 #305

Closed
damienwebdev opened this issue Jan 26, 2022 · 5 comments · Fixed by #307
Closed

Vet v6.0.0-alpha.5 types emitted types against v5.5.3 #305

damienwebdev opened this issue Jan 26, 2022 · 5 comments · Fixed by #307
Assignees
Labels
p: 3-urgent Fix and release ASAP

Comments

@damienwebdev
Copy link
Member

damienwebdev commented Jan 26, 2022

We need to ensure that the types emitted from our packages are API compatible with the previous DefinitelyTyped package.

See:

  1. types: random.arrayElement (and probably others) should support ReadonlyArray #166
  2. Accept dates as params for Date methods #198
  3. faker.address.nearbyGPSCoordinate (coordinate is a ReadOnlyArray in DefinitelyTyped)
  4. faker.address.companyName has an arg of format which should be string | number
  5. faker.database.column is implicitly return typed as string in 6.0.0-alpha.5 but should be set to return string
  6. faker.database.type() is implicitly return typed as string in 6.0.0-alpha.5 but should be set to return string
  7. faker.database.collation() is implicitly return typed as string in 6.0.0-alpha.5 but should be set to return string
  8. faker.database.engine() is implicitly return typed as string in 6.0.0-alpha.5 but should be set to return string
  9. faker.finance.currencyCode() is return typed any
  10. faker.finance.currencyName() is return typed any
  11. faker.finance.currencySymbol() is return typed any
  12. faker.hacker.abbreviation() is implicitly return typed as string
  13. faker.hacker.adjective() is implicitly return typed as string
  14. faker.hacker.noun() is implicitly return typed as string
  15. faker.hacker.verb() is implicitly return typed as string
  16. faker.hacker.ingverb() is implicitly return typed as string
  17. faker.hacker.phrase() is implicitly return typed as string

I'm sure there are a few others, but we need to check type by type to ensure spec compatibility.

@Shinigami92
Copy link
Member

It could help to look what the types were their, but keep in mind that also these could be out of sync from the source code and introduce false assumptions.
Don't just trust these blindly.

The Date type issue explicitly showed how wrong type assumption can nearly introduce new bugs to us without covered by the tests 😬

We are still in alpha and on some places I even did not assign a type yet, to explicitly find out what the real type is.

@Shinigami92 Shinigami92 self-assigned this Jan 26, 2022
@Shinigami92 Shinigami92 added p: 3-urgent Fix and release ASAP and removed s: pending triage Pending Triage labels Jan 26, 2022
@Shinigami92
Copy link
Member

Here is a version of the types: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/291c346588768b8ea13472ff95bdacd659f67790/types/faker/index.d.ts

Note that there are also some interfaces like https://github.com/DefinitelyTyped/DefinitelyTyped/blob/291c346588768b8ea13472ff95bdacd659f67790/types/faker/index.d.ts#L336

These could be helpful for https://github.com/faker-js/faker/pull/151/files#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R202, but I think I will split this PR into multiple smaller once anyway

@ST-DDT
Copy link
Member

ST-DDT commented Jan 26, 2022

Is there a tool to automatically compare the types?

@import-brain
Copy link
Member

Reopened due to my PR not fixing every single type listed here

@import-brain import-brain reopened this Jan 28, 2022
@Shinigami92
Copy link
Member

@damienwebdev Would you like to update the description of this issue and notify me/us about what is still missing?

@ST-DDT ST-DDT mentioned this issue Feb 22, 2022
5 tasks
@ST-DDT ST-DDT closed this as completed Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p: 3-urgent Fix and release ASAP
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants