Skip to content
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.

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

tracking: enable noUncheckedIndexedAccess #4040

Closed
36 tasks done
iuioiua opened this issue Dec 30, 2023 · 13 comments · Fixed by #4484
Closed
36 tasks done

tracking: enable noUncheckedIndexedAccess #4040

iuioiua opened this issue Dec 30, 2023 · 13 comments · Fixed by #4484
Labels
good first issue Good for newcomers PR welcome A pull request for this issue would be welcome v1

Comments

@iuioiua
Copy link
Collaborator

iuioiua commented Dec 30, 2023

Background

The noUncheckedIndexedAccess TSConfig option makes access objects safer and improves object inference. Enabling this option will improve the overall code quality of the Standard Library. This tracking issue aims to shine a new light on the functionality first brought up in #937.

Objectives

Modify the sub-modules of the Standard Library such that the TypeScript checker has no problems. The list of applicable sub-modules includes:

  • _tools
  • archive
  • assert
  • async
  • bytes
  • cli
  • collections
  • console
  • crypto
  • csv
  • data_structures
  • dotenv
  • encoding
  • expect
  • fmt
  • front_matter
  • fs
  • html
  • http
  • ini
  • io
  • json
  • jsonc
  • log
  • media_types
  • path
  • semver
  • streams
  • testing
  • text
  • toml
  • ulid
  • uuid
  • url
  • webgpu
  • yaml

How to contribute

This assumes that the Contributing Guidelines are being followed. Also, please read this before proceeding with one of these PRs.

  1. Choose a sub-module or small set of sub-modules you'd like to work on.
  2. Temporarily enable the option in the Deno configuration file:
    // deno.json
    {
      "compilerOptions": {
      "strict": true,
      "useUnknownInCatchVariables": true,
      "noImplicitOverride": true,
    + "noUncheckedIndexedAccess": true
      },
    ...
    }
  3. Make the necessary code modifications to your given sub-module(s) until the type-checker is happy. E.g. for std/collections:
    deno check collections/mod.ts
  4. Revert the option in the Deno configuration file.
    // deno.json
    {
      "compilerOptions": {
      "strict": true,
      "useUnknownInCatchVariables": true,
      "noImplicitOverride": true,
    - "noUncheckedIndexedAccess": true
      },
    ...
    }
  5. Submit the PR as per the Contributing Guidelines.

The option will be enabled globally once the appropriate adjustments have been made to the entire codebase.

References

@syhol
Copy link
Contributor

syhol commented Dec 31, 2023

The following seem to be free of issues:

  • datetime
  • msgpack
  • net
  • permissions
  • regexp

The following are used by many other modules so appear frequently as noise when running deno check [mod]/*.ts:

  • assert
  • path
  • streams
  • encoding

The following have many issues and might take a bit more effort:

  • encoding (79 issues, but also 17 from assert)
  • cli (60 issues, but also 17 from assert)
  • http (69 issues, but also 17 from assert, 54 from cli, 13 from testing, 13 from path, 19 from encoding)
  • data_structures (270 issues, but also 17 from assert)
  • testing (~200 issues)

I'm going to have a bash at assert next as it as nearly every other submodule depends on it in the testing code.

@iuioiua
Copy link
Collaborator Author

iuioiua commented Jan 1, 2024

Thanks, @syhol! I've updated the initial comment.

@syhol
Copy link
Contributor

syhol commented Jan 10, 2024

flags is passing the check now that cli has been resolved. This can be removed or checked off @iuioiua.

@javihernant
Copy link
Contributor

working on enabling noUncheckedIndexedAccess in path

javihernant added a commit to javihernant/deno_std that referenced this issue Feb 19, 2024
javihernant added a commit to javihernant/deno_std that referenced this issue Feb 19, 2024
javihernant added a commit to javihernant/deno_std that referenced this issue Feb 19, 2024
javihernant added a commit to javihernant/deno_std that referenced this issue Feb 20, 2024
javihernant added a commit to javihernant/deno_std that referenced this issue Feb 20, 2024
javihernant added a commit to javihernant/deno_std that referenced this issue Feb 20, 2024
iuioiua pushed a commit that referenced this issue Feb 20, 2024
refactor(path): prepare for noUncheckedIndexedAccess (#4040)
@iuioiua
Copy link
Collaborator Author

iuioiua commented Mar 5, 2024

For those wanting to contribute, I've updated the task list, as a few sub-modules have changed since the creation of this issue. Only a little more to go.

@syhol
Copy link
Contributor

syhol commented Mar 6, 2024

http was done by @gabelluardo on Feb 15th. I think you can check it off.

@iuioiua
Copy link
Collaborator Author

iuioiua commented Mar 7, 2024

There are still missing parts in http/unstable_signed_cookie.ts.

@syhol
Copy link
Contributor

syhol commented Mar 7, 2024

There are still missing parts in http/unstable_signed_cookie.ts.

I can't recreate it. @iuioiua is the issue on my side or yours? Could we get a third party to check?

Terminal output from my local setup
deno_std on  main [!] via 🦕 v1.41.1 git pull origin main
From https://github.com/denoland/deno_std
 * branch              main       -> FETCH_HEAD
Already up to date.

deno_std on  main [!] via 🦕 v1.41.1 git diff
diff --git a/deno.json b/deno.json
index 619ab66f..d2db1fcf 100644
--- a/deno.json
+++ b/deno.json
@@ -2,6 +2,7 @@
   "compilerOptions": {
     "strict": true,
     "useUnknownInCatchVariables": true,
+    "noUncheckedIndexedAccess": true,
     "noImplicitOverride": true
   },
   "imports": {

deno_std on  main [!] via 🦕 v1.41.1 deno check http/**/*.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/_mock_conn.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/_negotiation/common.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/_negotiation/encoding.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/_negotiation/language.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/_negotiation/media_type.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/cookie.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/cookie_test.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/etag.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/etag_test.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/file_server.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/file_server_test.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/mod.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/negotiation.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/negotiation_test.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/server.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/server_sent_event_stream.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/server_sent_event_stream_test.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/server_test.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/status.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/status_test.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/test.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/testdata/file_server_as_library.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/unstable_signed_cookie.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/unstable_signed_cookie_test.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/user_agent.ts
Check file:///Users/simon.holloway/Code/syhol/deno_std/http/user_agent_test.ts
Command to run it inside a fresh checkout in a docker container

Warning: Its a bit slow, takes about 3-4 minutes to run for my machine

docker run -i --rm denoland/deno:1.41.1 bash << EOF
  apt-get update
  apt-get install git jq -y
  cd srv
  git clone https://github.com/denoland/deno_std
  cd deno_std

  git pull origin main
  jq '.compilerOptions += {"noUncheckedIndexedAccess": true}' deno.json > deno-new.json
  mv deno-new.json deno.json
  git diff
  find http -type f -name "*.ts" | xargs -n1 deno check && echo "Check Succeeded" || echo "Check Failed" 
EOF

@iuioiua
Copy link
Collaborator Author

iuioiua commented Mar 7, 2024

Use deno task test instead of deno check. I submitted #4453 with the changes.

@syhol
Copy link
Contributor

syhol commented Mar 7, 2024

Ahh I see thanks.

@iuioiua
Copy link
Collaborator Author

iuioiua commented Mar 11, 2024

I've just updated the list once more. We're now quite close to having this completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers PR welcome A pull request for this issue would be welcome v1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants