Skip to content

Improve Deno project - #111

Merged
mxdvl merged 3 commits into
mainfrom
mxdvl/improve-deno-project
Feb 13, 2024
Merged

mxdvl merged 3 commits into
mainfrom
mxdvl/improve-deno-project

Conversation

@mxdvl

@mxdvl mxdvl commented Feb 6, 2024

Copy link
Copy Markdown
Contributor

What does this change?

  • Better & simpler docs
  • Stricter typing

How to test

deno task test

How can we measure success?

Up-to-date project

Have we considered potential risks?

N/A

@mxdvl mxdvl changed the title Improve deno project Improve Deno project Feb 6, 2024
Comment thread typescript-deno/deno.jsonc Outdated
// https://github.com/guardian/csnx/blob/1be3fd98ed527ccf0f1b8e21f10039e23fdb3fb7/libs/%40guardian/tsconfig/tsconfig.json
"compilerOptions": {
"noImplicitReturns": true,
"noUncheckedIndexedAccess": true,

@marsavar marsavar Feb 7, 2024 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have a feeling this specific compiler option might create more friction than intended, as it is known to create false positives. The TypeScript development team have repeatedly resisted calls to make this rule a default due to its inflexibility; see here.

This appears to be an issue particularly when attempting to index into arrays in bounds-checked loops, see example in the docs

function screamLines(strs: string[]) {
  // This will have issues
  for (let i = 0; i < strs.length; i++) {
    console.log(strs[i].toUpperCase());
   // Object is possibly 'undefined'.
  }
}

My experience doing coding exercise interviews is that candidates often reach for C-style for loops and this rule might end up creating more confusion than needed. I would therefore skip it if possible, but happy to hear your thoughts.

The other rules (noImplicitReturns and noUnusedLocals) seem like great additions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That’s a fair point… Do we think that this is an interesting point of discussion for interviews?


For any future candidate reading this, I’m sure you’ll make a good impression if you do this instead 😉:

function screamLines(strs: string[]) {
  for (const str of strs) {
    console.log(str.toUpperCase());
  }
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the game of life exercise it's extremely common to use indexing into a 2d array to find the neighbours in a grid, and it raises interesting questions about how to do it safely. For accessing neighbours in a 2d array of arrays how would you do that whilst keeping this rule happy?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@michaelclapham simply by checking whether the value is not undefined.

const grid = [
  [1, 1, 0],
  [0, 0, 0],
  [1, 0, 1],
];

const safe = grid[0]?.[0] // number | undefined
if (typeof safe === "number") {
  console.log("we know this is a safe lookup!", safe)
}

const unsafe = grid[9]?.[9] // number | undefined
if (typeof unsafe === "number") {
  console.log("This will never happen!", unsafe)
}

Playground Link

This can easily throw errors that may
not be valuable in the context of a code
pairing test for our candidates

Co-authored-by: Mario Savarese <mario.savarese@theguardian.com>
@mxdvl
mxdvl requested a review from marsavar February 7, 2024 21:09

@michaelclapham michaelclapham left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overall looks good!

@mxdvl
mxdvl merged commit 7728d84 into main Feb 13, 2024
@mxdvl
mxdvl deleted the mxdvl/improve-deno-project branch February 13, 2024 15:41
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.

3 participants