Skip to content

Improve Deno project #111

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 13, 2024
Merged

Improve Deno project #111

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3deb103
docs(deno): improve README
mxdvl Feb 3, 2024
8c7438d
feat(deno): stricter compiler options
mxdvl Feb 3, 2024
eb90c8b
fix: allow unchecked indexed access
mxdvl Feb 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 5 additions & 10 deletions typescript-deno/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,16 @@ If using VSCode, you might want to enable the extension in the settings:
```jsonc
// .vscode/settings.json
{
"deno.enable": true
"deno.enable": true,
// or specifically
"deno.enablePaths": ["./typescript-deno"]
}
```

## Usage

- `./script/start` to run your code
- `./script/start` to run the tests

### Other commands

- `deno run src/exerciste.ts --watch` will re-run [`mod.ts`](./src/mod.ts) on every
file change, for quick development feedback.
- `deno test src --watch` will re-run [`mod.test.ts`](./src/mod.test.ts) on every file
change, for quick development feedback.
- `./script/start` to run the source code
- `./script/test` to run the tests in watch mode

## Structure

Expand Down
13 changes: 13 additions & 0 deletions typescript-deno/deno.jsonc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"tasks": {
"start": "./script/start",
"test": "./script/test"
},
// https://github.com/guardian/csnx/blob/1be3fd98ed527ccf0f1b8e21f10039e23fdb3fb7/libs/%40guardian/tsconfig/tsconfig.json
"compilerOptions": {
"noImplicitReturns": true,
"noUncheckedIndexedAccess": true,
Copy link
Contributor

@marsavar marsavar Feb 7, 2024 •
edited
Loading

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
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
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
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

"noUnusedLocals": true,
"strict": true
}
}
9 changes: 9 additions & 0 deletions typescript-deno/deno.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion typescript-deno/script/test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@

set -e

deno test src
deno test src --watch