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

feat: support fields slice variation config #337

Closed
wants to merge 8 commits into from

Conversation

angeloashmore
Copy link
Member

@angeloashmore angeloashmore commented Apr 9, 2024

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 adds support for the fields slice variation config. fields is an upcoming feature that allows slices to use group fields.

  • Add a fields property to SharedSliceModelVariation.
  • Add a data property to SharedSliceVariation.
  • Add a SharedSliceVariationWithData which represents a slice using the data property.
  • Add a SharedSliceVariationWithPrimaryAndItems which represents a slice using the primary and items properties.

The fields config is preferred over the primary and items config. primary and items will continue to be supported, but new slices will use fields.

Related: DT-2078

Backward compatibility

There are no breaking changes in this PR. Some of the implementation may look strange, like SharedSliceVariation. Some concessions were necessary to be backward compatible with the current API.

prismic-ts-codegen will be updated to use SharedSliceVariationWithData and SharedSliceVariationWithPrimaryAndItems only (i.e. no use of SharedSliceVariation), which will prevent users from interacting with slightly inaccurate types.

Example

type CallToActionSliceModel = SharedSliceModel<
	"call_to_action",
	SharedSliceModelVariation<
		"default",
		{
			body: CustomTypeModelRichTextField;
		}
	>
>;

type CallToActionSlice = SharedSlice<
	"call_to_action",
	SharedSliceVariationWithData<
		"default",
		{
			body: RichTextField;
		}
	>
>;

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.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.97%. Comparing base (8b20c5a) to head (cc515ce).

❗ Current head cc515ce differs from pull request most recent head f65f07b. Consider uploading reports for the commit f65f07b to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #337   +/-   ##
=======================================
  Coverage   99.97%   99.97%           
=======================================
  Files          61       61           
  Lines        6905     6924   +19     
  Branches      381      381           
=======================================
+ Hits         6903     6922   +19     
  Misses          2        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@angeloashmore angeloashmore marked this pull request as ready for review April 10, 2024 03:51
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 as per the current specs however please ignore it all depending on the outcome of this discussion: https://prismic-team.slack.com/archives/C02L3FN3AJK/p1712751482676019

Also I get it we want models to be defined with fields and value with data. On custom types they call it json (which I think is a weird name) instead of fields so it looks like we have no standards here, I'd be happy with fields being the standard moving forward.

@@ -4,6 +4,7 @@ import type { ContentRelationshipField } from "./contentRelationship";
import type { DateField } from "./date";
import type { EmbedField } from "./embed";
import type { GeoPointField } from "./geoPoint";
import { GroupField } from "./group";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { GroupField } from "./group";
import type { GroupField } from "./group";

Comment on lines +40 to +51
/**
* A shared slice variation.
*/
export type SharedSliceVariation<
Variation extends string = string,
Fields extends Record<string, SliceField> = Record<string, SliceField>,
ItemsFields extends Record<string, AnyRegularField> = Record<
string,
AnyRegularField
>,
> = SharedSliceVariationWithData<Variation, Fields> &
SharedSliceVariationWithPrimaryAndItems<Variation, Fields, ItemsFields>;
Copy link
Member

Choose a reason for hiding this comment

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

This type is more of an internal type right (in the spirit)? I don't really see how the returned type could be helpful to users since it awkwardly has both primary and data with primary potentially containing illegal group fields 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

This type was intended to be a backward-compatible version since all current generated types use SharedSliceVariation. Future type generations would use the more specific types instead. So not really "internal," but definitely temporarily used.

Anyway, we are going away from this PR so we can avoid this weird, dirty type. 😅

@angeloashmore
Copy link
Member Author

Closing in favor of #338.

We decided not to use fields/data and instead only add support for group fields in the existing primary property. The change in direction greatly simplifies the feature.

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

3 participants