prismicio / prismic-types

Type definitions for Prismic content, models, and APIs
https://prismic.io/docs/technologies/javascript
Apache License 2.0
11 stars 4 forks source link

feat!: update model type definitions to match `@prismicio/types-internal` #43

Closed lihbr closed 2 years ago

lihbr commented 2 years ago

This is an attempt as making sure external type definitions for models match internal ones, but I kinda hate it 🤔

From what I experienced:

As stated in the branch name, this is really just an experimental PR, it caught a few errors at least (namely, placeholder_true/placeholder_false on external boolean model type)

codecov-commenter commented 2 years ago

Codecov Report

Merging #43 (b88f47d) into master (f7c39e3) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #43   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines         1698      1768   +70     
  Branches         7         7           
=========================================
+ Hits          1698      1768   +70     
Impacted Files Coverage Δ
src/customType.ts 100.00% <100.00%> (ø)
src/index.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f7c39e3...b88f47d. Read the comment docs.

lihbr commented 2 years ago

OK, so I refactored the PR following our discussion so that we only focus on having external types (these types) being compatible with internal types.

const fn = (obj: ExternalType) => {};

const internal: InternalType = {};
const external: ExternalType = {};

fn(internal); // OK
fn(external); // OK

I tried to shave it down to what made sense, however, I couldn't get rid of:

Let me know if you think those updated types seem manageable by prismic-ts-codegen and if you have any better idea on how to better hide the 3 above~

lihbr commented 2 years ago

Alright, fixed style and reverted back readonly's as discussed above.

Right now @prismicio/types are properly compatible with object types with @prismicio/types-internal, not the other way around. We'll add test back for those when we get @prismicio/types object compatible with @prismicio/types-internal.

arnaudlewis commented 2 years ago

Ongoing draft PR on types-internal to simplify the integration: https://github.com/prismicio/prismic-types-internal/pull/3

lihbr commented 2 years ago

Ongoing draft PR on types-internal to simplify the integration: prismicio/prismic-types-internal#3

Added back two-ways test with 0b872c7, tested with https://github.com/prismicio/prismic-types-internal/pull/3 and everything passes cleanly 🙌

edit: I don't get why types is passing in the CI since it doesn't have the PR yet 🤔 https://github.com/prismicio/prismic-types/runs/7090783719?check_suite_focus=true#step:7:1, locally it's not passing without the PR

angeloashmore commented 2 years ago

I corrected a small error in a test and made the link field tests more specific.

I'm going to merge and publish this PR. We can adjust the link field tests later if there's something we need to change.