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

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

Merged
merged 4 commits into from
Jun 3, 2023

Conversation

angeloashmore
Copy link
Member

@angeloashmore angeloashmore commented Jun 1, 2023

Types of changes

  • Chore (a non-breaking change which is related to package maintenance)
  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

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

  • PrismicDocument.first_publication_date
  • PrismicDocument.last_publication_date

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:

  • My change requires an update to the official documentation.
  • All TSDoc comments are up-to-date and new ones have been added where necessary.
  • All new and existing tests are passing.

🦒

Copy link
Member

@lihbr lihbr left a comment

Choose a reason for hiding this comment

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

LGTM!

@angeloashmore
Copy link
Member Author

angeloashmore commented Jun 2, 2023

@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
Copy link

codecov-commenter commented Jun 2, 2023

Codecov Report

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

❗ 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           

Copy link
Member

@lihbr lihbr left a comment

Choose a reason for hiding this comment

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

All good dropping Node 14!

For the @ts-expect-error, maybe let's just not add them, ignore the CI & publish manually (bypassing part of the release script)?

@angeloashmore angeloashmore merged commit 7cce22a into master Jun 3, 2023
@angeloashmore angeloashmore deleted the aa/publication-timestamps branch June 3, 2023 02:13
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.

3 participants