-
Notifications
You must be signed in to change notification settings - Fork 62
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
Conversation
There was a problem hiding this 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
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Codecov Report
❗ 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
|
Thanks for catching the clearly incorrect TSDocs! Can you tell I copied it over from the initial The types should not have been exported. I also changed the loose
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 What do you think though? Should we just commit to adding it to |
There was a problem hiding this 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!
Types of changes
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 singlecodeHTML
property. The property contains HTML produced from ahighlight()
function, which could be a function that calls a syntax highlighting library, like Shiki.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 theslice
object. Other data is included, such asslices
, andindex
.A third parameter (
context
) can be provided tounstable_mapSliceZone()
, whose contents will be passed to the function in thecontext
property. The context object is most helpful when mapping functions are defined outside theunstable_mapSliceZone()
calling scope, such as in a different module.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:
🦦