prismicio / prismic-client

The official JavaScript + TypeScript client library for Prismic
https://prismic.io/docs/technical-reference/prismicio-client
Apache License 2.0
168 stars 60 forks source link

fix: type `PrismicDocument.*_publication_date` as `TimestampField<"filled">` #304

Closed angeloashmore closed 1 year ago

angeloashmore commented 1 year ago

Types of changes

Description

This PR changes the following fields from string to TimestampField<"filled">:

Retyping the properties makes them compatible with asDate() when type-checked.

Requested by @chamois-d-or

Discussion points

Will this have negative downstream effects?

Changing the types makes the properties more specific, which shouldn't affect user code that accepted one of the properties. It could, however, cause issues in edge cases where user code was overriding the properties.

Is there a better way to install @prismicio/client (i.e. itself) as @prismicio/mock's peer dependency?

@prismicio/mock was updated with a peer dependency on @prismicio/client. npm was installing the published version alongside mock, which did not include the updated types.

Because this PR changes PrismicDocument types, it caused type incompatibilities between @prismicio/mock and src/ version of @prismicio/client.

I was able to solve the issue by pointing the @prismicio/client dependency on itself via "@prismicio/client": ".", which is a very odd setup.

Checklist:

🦒

angeloashmore commented 1 year ago

@lihbr Could you take one more look? The most recent commit (see here) shows how I removed "@prismicio/client": "." from package.json.

Doing so required using @ts-expect-error, which should be removable once we merge and publish this version.

CI fails because (I assume) @prismicio/client is not being automatically installed via @prismicio/mock's peer dependency on the package. I would prefer not adding @prismicio/client@latest to package.json since that means we would need to maintain that version. Should we remove Node.js 14 (EOL) from our list of runners?

codecov-commenter commented 1 year ago

Codecov Report

Merging #304 (d8b7aff) into master (db85d21) will not change coverage. The diff coverage is n/a.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##           master     #304   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files          49       49           
  Lines        5764     5764           
  Branches      309      308    -1     
=======================================
  Hits         5761     5761           
  Misses          3        3