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: add unstable_mapSliceZone() #302

Merged
merged 4 commits into from
Jun 7, 2023
Merged

feat: add unstable_mapSliceZone() #302

merged 4 commits into from
Jun 7, 2023

Conversation

angeloashmore
Copy link
Member

@angeloashmore angeloashmore commented May 31, 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 adds an experimental unstable_mapSliceZone() helper. The helper maps a Slice Zone's Slices using a set of mapping functions, one for each type of a Slice Zone's Slice.

Developers often need to transform or augment Slices when building relatively complex websites. We currently do not offer an official solution for this need, thus developers come up with their own non-standard solutions.

This function is primarily recommended for use on the server where Slices can be augmented, reshaped, or trimmed-down.

The solution provided in this PR is general, flexible, and tested.

Example

The following example reshapes Code Block Slices (id: code_block) by replacing it with an object containing a single codeHTML property. The property contains HTML produced from a highlight() function, which could be a function that calls a syntax highlighting library, like Shiki.

async function highlight(code: string, language: string): Promise<string> {
  // Process the code and return HTML.
}

const mappedSliceZone = await unstable_mapSliceZone(page.data.slices, {
  code_block: async ({ slice }) => ({
    codeHTML: await highlight(slice.primary.code, slice.primary.language),
  }),
});

Any occurance of a Code Block Slice will be replaced with the mapped version. Note how the function provided to the code_block property receives the slice object. Other data is included, such as slices, and index.

A third parameter (context) can be provided to unstable_mapSliceZone(), whose contents will be passed to the function in the context property. The context object is most helpful when mapping functions are defined outside the unstable_mapSliceZone() calling scope, such as in a different module.

const mappedSliceZone = await unstable_mapSliceZone(
  page.data.slices,
  {
    code_block: async ({ slice, context }) => ({
      codeHTML: await highlight(slice.primary.code, slice.primary.language),
    }),
  },
  context
);

Unstable

This helper is marked as unstable_ in its function name and @experimental in its TSDocs. It is being added as a way for developers to test it in production and provide feedback before being marked as stable.

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.

🦦

@angeloashmore angeloashmore requested a review from lihbr May 31, 2023 02:12
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.

Made some suggestions, but actually, are you also using this PR to export and reuse looser types for <SliceZone /> component implementations? 🤔

I'm not sure about the _unstable prefix, is it that unstable, as in, is there still chances for us to get rid of it? If yes, then sure, let's keep it, otherwise I'd consider dropping it.

src/helpers/unstable_mapSliceZone.ts Outdated Show resolved Hide resolved
src/helpers/unstable_mapSliceZone.ts Outdated Show resolved Hide resolved
src/helpers/unstable_mapSliceZone.ts Outdated Show resolved Hide resolved
src/helpers/unstable_mapSliceZone.ts Outdated Show resolved Hide resolved
src/helpers/unstable_mapSliceZone.ts Outdated Show resolved Hide resolved
src/helpers/unstable_mapSliceZone.ts Outdated Show resolved Hide resolved
src/helpers/unstable_mapSliceZone.ts Outdated Show resolved Hide resolved
Comment on lines 239 to 247
let result = await mapper(mapperArgs);

if (
mapper.length < 1 &&
(typeof result === "function" ||
(typeof result === "object" && "default" in result))
) {
result = "default" in result ? result.default : result;
result = await result(mapperArgs);
Copy link
Member

Choose a reason for hiding this comment

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

I'd maybe rename the first result to something like resultOrMapperModule so that the following line can read better.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer result here since we ultimately will be using result as the final mapper function return value. result is used later in the function to spread into an object, which should never be a module.

However, I added comments around const result to explain exactly what is happening at that stage. I think that should accomplish what your rename suggestion is doing.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Works for me!

* If `true`, this Slice has been modified from its original value using a
* mapper.
*/
__mapped: true;
Copy link
Member

Choose a reason for hiding this comment

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

Can we @internal it so it's more likely to be hidden from intellisense?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if we should mark it as @internal. We need to expose it so consumers of the Slice Zone can use it. It is a public way to identify if a Slice has been mapped.

We initially only plan to use it within <SliceZone> so we can determine how to pass a component its props, but there may be other use cases for mapping a Slice Zone.

test/helpers-unstable_mapSliceZone.test.ts Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2023

Codecov Report

Merging #302 (58d7e65) into master (7cce22a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ 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     #302    +/-   ##
========================================
  Coverage   99.94%   99.95%            
========================================
  Files          49       50     +1     
  Lines        5764     6034   +270     
  Branches      308      319    +11     
========================================
+ Hits         5761     6031   +270     
  Misses          3        3            
Impacted Files Coverage Δ
src/helpers/unstable_mapSliceZone.ts 100.00% <100.00%> (ø)
src/index.ts 100.00% <100.00%> (ø)

@angeloashmore
Copy link
Member Author

Made some suggestions, but actually, are you also using this PR to export and reuse looser types for <SliceZone /> component implementations? 🤔

Thanks for catching the clearly incorrect TSDocs! Can you tell I copied it over from the initial @prismicio/react implementation? 😅

The types should not have been exported. I also changed the loose SliceLike type to Slice, but kept SliceGraphQLLike since this function should support the GraphQL API where we don't always know the shape of the object.

I'm not sure about the _unstable prefix, is it that unstable, as in, is there still chances for us to get rid of it? If yes, then sure, let's keep it, otherwise I'd consider dropping it.

unstable_ is a React convention to mark a function as volatile. Its implementation or APIs may change and should be considered unfinished. Once we determine that this function works well and is useful, we can remove the unstable_ prefix and deem it stable.

This function designed for a common, but advanced, use case, so I'm not 100% sure it should be treated the same as our other helpers, like asDate() and asLink(). For now, we can safely treat it as an experiment with no downsides.

What do you think though? Should we just commit to adding it to @prismicio/client as a stable helper?

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, not sure what's going on with the types though 🤔

And OK, no, I'm fine with keeping it as unstable_ for now, nice convention, didn't know about it!

@angeloashmore
Copy link
Member Author

The types are failing due to the changes in #304. Once we publish #304, which I'll include as part of publishing this PR, the type errors should go away.

@angeloashmore angeloashmore merged commit 9e604d3 into master Jun 7, 2023
7 of 12 checks passed
@angeloashmore angeloashmore deleted the aa/slice-mapper branch June 7, 2023 22:57
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