Add more TypeScript guidelines #7407
Merged
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
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Hey folks, I mentioned I'd share some more guidelines that have worked well for another project, and here they are! Some of these certainly won't fit this project, so please leave candid feedback. We can clean them up and land whatever works 🙂 |
|
Size Change: 0 B Total Size: 1.25 MB ℹ️ View Unchanged
|
sirreal
commented
Oct 6, 2023
Jinksi
reviewed
Oct 12, 2023
Jinksi
reviewed
Oct 13, 2023
Jinksi
approved these changes
Oct 13, 2023
There was a problem hiding this comment.
Thanks for those great additions @sirreal, I embrace all of them! 🙂
➕ Thanks for sharing these, it's great to collaborate and leverage your previous efforts and expertise.
We can probably put most of them in a
General conventionsfile, or something similar, at the top of the contents table.
Yep, this sounds good. We will have more guidelines in the near future, so we can iterate on organising these docs as we go.
|
We should totally have merged this but it dropped off the radar. I'll resurrect it 🌅 |
Jinksi
added
pr: ready to merge
type: documentation
|
@ismaeldcom @reykjalin would you like a final look at this helpful doc before we merge? |
|
Oh, I also missed this! Looks ready to merge to me. |
reykjalin
requested changes
Apr 11, 2024
Comment on lines
164
to
167
| ## Use const assertions | ||
|
|
||
| Not everything needs a _const assertion,_ but when we want to infer a readonly interface it's a | ||
| great option. Here's what that might look like: |
There was a problem hiding this comment.
I think we should add a bit more context on what const assertions are (I couldn't make a GH suggestion because the example has markdown code blocks 🙄 )
Use const assertions
const assertions are an excellent tool to prevent TS from widening inferred types. This can sound a bit abstract so consider the following code:
function getShapes() {
let result = [
{ kind: "circle", radius: 100 },
{ kind: "square", sideLength: 50 },
];
return result;
}
function useRadius( radius: number ) {
return radius;
}
for ( const shape of getShapes() ) {
if ( shape.kind === "circle" ) {
// TS still thinks shape can be any of the items returned from 'getShapes()' and thus (correctly) infers that 'shape.radius' may be 'undefined'.
useRadius( shape.radius ); // ⛔️ Can't pass 'number | undefined' when the function expects a 'number'.
}
}const assertions allow us to get a concrete type without resorting to type guards or type assertion:
function getShapes() {
let result = [
{ kind: "circle", radius: 100 },
{ kind: "square", sideLength: 50 },
] as const;
return result;
}
function useRadius( radius: number ) {
return radius;
}
for ( const shape of getShapes() ) {
if ( shape.kind === "circle" ) {
useRadius( shape.radius ); // ✅ Ok, TS knows that if kind === 'circle' then 'shape' must have a 'radius' prop!
}
}Not everything needs a const assertion, but when we want to infer a readonly interface it's a
great option. Here's what that might look like:
[…]
There was a problem hiding this comment.
I added this in e43cb6a, would love a review on the language used 🙂
Jinksi
approved these changes
Apr 12, 2024
There was a problem hiding this comment.
Great addition @reykjalin, definitely helps me understand this benefit of const assertions 💯
reykjalin
approved these changes
Apr 12, 2024
naman03malhotra
pushed a commit
that referenced
this pull request
Apr 15, 2024
Co-authored-by: Eric Jinks <[email protected]> Co-authored-by: Kristófer R <[email protected]> Co-authored-by: Kristófer Reykjalín <[email protected]>
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.
Changes proposed in this Pull Request
This is a follow-up to #7046 that adds more TypeScript guidelines.
These are guidelines that have been helpful for another TypeScript project (Tumblr). They may not all be applicable here. I've added them all in a single file for now for the purposes of discussion. They can be split into separate files when the desired guidelines are established.
Follow-up to #7046 (review)
npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge