Introduce RenderingTarget type and CombinedProps pattern
#7359
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
OllysCoding
commented
Mar 8, 2023
Comment on lines
+123
to
+124
| 'no-shadow': 'off', // We use the typescript-eslint version as eslint false positives on enums | ||
| '@typescript-eslint/no-shadow': ['error'], |
There was a problem hiding this comment.
Without making this change, we'd getting a variable is already declared in upper scope warning from ESLint, despite that not being the case. Using the typescript version of this rule fixes this problem
There was a problem hiding this comment.
Was this needed even without the use of TypeScript enums?
OllysCoding
commented
Mar 8, 2023
| @@ -125,10 +125,10 @@ export const FormField = ({ | |||
| }, | |||
| ] | |||
| .concat(formField.options) | |||
| .map(({ value, label }) => { | |||
| .map(({ value, label: formLabel }) => { | |||
There was a problem hiding this comment.
Fix a 'no shadow' error
OllysCoding
commented
Mar 8, 2023
| const nextOffset = offsets | ||
| .reverse() | ||
| .find((offset) => offset < scrolled); | ||
| const nextOffset = offsets.reverse().find((o) => o < scrolled); |
There was a problem hiding this comment.
Fix a 'no shadow' error
There was a problem hiding this comment.
Could also drop the reverse with Array.prototype.findLast if it’s polyfilled?
Just mentioning as reverse modifies the array in place
|
Size Change: -1.02 kB (0%) Total Size: 515 kB
ℹ️ View Unchanged
|
⚡️ Lighthouse report for the changes in this PRReport for Article
|
⚡️ Lighthouse report for the changes in this PRReport for Front
|
JamieB-gu
approved these changes
Mar 8, 2023
bryophyta
reviewed
Mar 9, 2023
|
|
||
| /** | ||
| * This type can be set to either AppsProps or WebProps, and takes two generics | ||
| * one for each rendering targets props |
There was a problem hiding this comment.
I know that we're not exporting this, but I think it would be useful to elaborate a bit on how this type works? (I think I know, but I'm not super confident on the details, and it feels like something that would be useful for future travellers.)
bryophyta
approved these changes
Mar 9, 2023
georgeblahblah
approved these changes
Mar 9, 2023
mxdvl
reviewed
Mar 9, 2023
Comment on lines
1
to
15
| /** | ||
| * An enum defining what where we are targeting a particular rendered | ||
| * page to be shown | ||
| * | ||
| * This can be used to make decisions during rendering, where there | ||
| * might be differences in the requirements of each target. | ||
| * | ||
| * Targets: | ||
| * - Web => A full web browser, such as chrome or safari on a desktop computer, laptop or phone. | ||
| * - Apps => A webview rendered within the Android or iOS live apps | ||
| */ | ||
| export enum RenderingTarget { | ||
| Web, | ||
| Apps, | ||
| } |
There was a problem hiding this comment.
As I mentioned in the RFC, I would recommend avoiding TypeScript enums, as they “are one of the few features TypeScript has which is not a type-level extension of JavaScript”1.
To prove my point, what do you expect the value of RenderingTarget[0] to be? What about RenderingTarget.Web === 0?
Footnotes
There was a problem hiding this comment.
Would your suggestion be to use a string union? What's the best alternative
There was a problem hiding this comment.
Yes, string unions are a lot less confusing. If you need to do custom type-guard, the pattern I shared in the RFC works well:
// If you define a array `as const`… const renderingTarget = [ "Web", "Apps", "Editions", ] as const; // … you can infer the type from it… export type RenderingTarget = renderingTarget[number]; // … and create a custom type guard, if you so desire! export isRenderingTarget = (target: string): target is RenderingTarget => renderingTarget.map(String).includes(target)
mxdvl
reviewed
Mar 9, 2023
There was a problem hiding this comment.
This looks like it bring a lot if type safety and is well documented: great!
Should the no-shadow change be extracted out of this PR?
Is the complexity of this wrapper type worth it compared to a naive implementation?
interface CommonProps {
value: string;
target: RenderingTarget; // enum or string union
}
interface AppProps extends CommonProps {
target: "Apps";
apiUrl: string;
}
interface WebProps extends CommonProps {
target: "Web";
}
const Component = (props: AppProps | WebProps) =>
<>{/* … */}</>
JamieB-gu
approved these changes
Mar 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on the RFC #7174, this PR creates the
RenderingTargettype andCombinedPropspattern.DCR & AR are becoming one - specifically we are adding the capability for DCR to render articles for the app.
We will continue to use the Frontend model for both web & apps articles, and generally web and apps articles will be the same. However, there are some differences in requirements, mainly based on the technical impact of the target:
To support these differences, this PR introduces the idea of a
RenderingTarget, an enum which allows a component or function to know whether the content being rendered will be shown either on the web or the apps.Additionally, to support differing requirements of data - a rare but nonetheless necessary occurrence - this PR introduces the
CombinedPropstype & pattern which allows the passing of 'web only' or 'apps only' props, ensuring our type safety is not diluted when rendering for different targets.Most of the additional details & reasoning are outlined in the RFC: #7174
An important part of this PR is the JSDoc documentation, so please provide feedback of the wording can be improved or better formatted!