Repository navigation
Conversation
195eb41 to
e8aeb0d
Compare
⚡️ Lighthouse report for the changes in this PRLighthouse tested 2 URLs Report for Article
Report for Front
|
|
Size Change: +982 B (0%) Total Size: 1.64 MB
ℹ️ View Unchanged
|
e8aeb0d to
4351d60
Compare
|
What's the overall difference between this & something like I understand there seems to be some typing differences, is |
| import { nonEmpty } from '../web/lib/tuple'; | ||
| import { enhanceImages } from './enhance-images'; | ||
|
|
||
| if (!nonEmpty(images)) throw new Error('Empty images list'); |
There was a problem hiding this comment.
I find it exceedingly frustrating that we're having to add checks here to what is a static array declared in code. Putting aside whether this overall tuple pattern is necessary for the code, can we update our fixtures to use as const or find another way to ensure we don't need to check them?
There was a problem hiding this comment.
as const is definitely an approach we could use, but it would require more refactoring that a single line, because you then need the consumers to receive readonly arrays. I have given it a go and it is non-trivial, which is why I opted for a single error that should “never occur”.
Even though these arrays are fixed in our code, it’s worth noting that a change in the way the fixtures are generated could mean that in the future we have more or less items, and our tests would start failing. If this happens, the developers will not get an explicit prompt of what failed. By adding this check, we ensure that such a failure will be as explicit as possible.
There was a problem hiding this comment.
Here’s a propsal: https://github.com/guardian/dotcom-rendering/pull/5817/files
| html: '<p>Just some normal text</p>', | ||
| }; | ||
|
|
||
| if (!(images[0] && images[1] && images[2] && images[3])) { |
There was a problem hiding this comment.
Should this be isTuple?
There was a problem hiding this comment.
Not quite, because we have more than 4 elements, we only care that the first four are defined.
You can omit the images[0], but that’s about it.
The main difference is that we are creating type-guards for the compiler:
|
3cc3246 to
1db7bbf
Compare
These type-guards can prevent unchecked indexed accesses
This helps narrow many of the arrays in our codebase.
1db7bbf to
cde8986
Compare
|
Are we making our code more complex by adding a rule that is guarding against a problem that we don't really have? Both this PR using new helper functions, and the alternative with direct checks, add complexity to the code. I don't think we should be adding non idiomatic language features into DCR but the alternative type checks shown in the other PR make the code much harder to follow. This complexity has a real cost, not just in terms of the time it takes to implement this rule but the much larger cost of the DCR repo being harder to work on moving forward. Every time anyone needs to access an array they will need to either learn these abstractions or jump through type hoops. In particular, the fronts container code feels like it would be especially impacted here which is something I think we should be cautious about. But how often does Frontend send us arrays like Perhaps we can look at solving any problems we've seen locally without a global rule? If we're using |
|
Thanks for raising your concerns @oliverlloyd – I came to propose this change as I believe it solves some real business issues, so will try and articulate some of the benefits as answers to your points above. 👇
It is hard to say whether we have these problems currently, as this is mainly used for containers that are not used in production. Currently, the burden of safety is on the implementation of these components, and we are relying on I have found the Dynamic container PRs hard to review and would feel quite uneasy refactoring their logic. I found introducing tuples retraining the number of cards a component accepts both freeing and simpler, hence my proposal to introduce tuples more broadly.
This is an excellent point and I really was hoping that these abstractions were making things easier, not harder. With the array type, any specific length requirement has to be expressed in the JS logic—like we currently do. This means that the interface of our components is neither an accurate representation of what they truly need, nor is it preventing the logic from drifting away in the future. In general, this means pushing the error cases to the edge of the application, and greatly reducing checks at the deepest levels. Instead of forcing components to handle any size of arrays when they can really only ever render, say, 4 items, we can use the compiler to enforce this all the way up the stack.
Not all arrays need to be checked, only some specific cases where we actually expect a fixed length.
Looking at logs of 500, I could only find one example: I don’t know how you balance errors v safety, but I like to know that these kinds of bugs can be avoided.
I think the main benefit of TypeScript is preventing runtime errors and this is a real that can happen, so I would be enclined to turn it on? |
|
Moving this back to draft, as we need to come up with better recommendations around introducing new TypeScript types. |
What does this change?
Introduce two tuple helpers which help narrowing types:
nonEmptyandisTuple.By checking an array is
nonEmpty, you can ensure that there is at least a single element.By checking that an array
isTuple(…, n), you can ensure that there is preciselynelements in it.Why?
These will throw errors when we have
noUncheckedIndexedAccessin #5777.When we try to access an element in an array, the TypeScript compiler will not warn us by default that the element might be
undefined. There are two very common cases in our codebase, and these helpers cover them: checking that an array has a least one item, and checking that an array has a precise length.These patterns are used a lot in the Fronts cards, forcing precise number of cards where we expect them, instead of failing softly like we do now.