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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Outdated documentation links for tsetse errors #2367

Open
tony-scio opened this issue Dec 23, 2020 · 7 comments
Open

Outdated documentation links for tsetse errors #2367

tony-scio opened this issue Dec 23, 2020 · 7 comments
Assignees
Labels
cleanup Tech debt, resolving it improves our own velocity help wanted question/docs Answer users question, preferably by updating docs

Comments

@tony-scio
Copy link
Contributor

馃悶 bug report

Affected Rule

The issue is caused by the rule: ts_library

Is this a regression?

Yes, the previous version in which this bug was not present was: Presumably the links worked at some point

Description

After v3.0.0 upgrade, I started getting errors like:
error TS21231: [tsetse] type assert `JSON.parse() as SomeExplicitType` for type & optimization safety.
	See http://tsetse.info/must-type-assert-json-parse.

157   return Array.from(newFilters.entries()).map(([k, v]) => ({ ...JSON.parse(k), values: v }))
  1. http://tsetse.info/must-type-assert-json-parse is broken.
  2. https://tsetse.info/ says it moved to https://github.com/bazelbuild/rules_nodejs/tree/3.x/third_party/github.com/bazelbuild/rules_typescript, but clicking the message goes to https://github.com/bazelbuild/rules_typescript
  3. That pages says it moved to https://github.com/bazelbuild/rules_nodejs/tree/3.x/third_party/github.com/bazelbuild/rules_typescript, but clicking that gives a 404.

A google search for TS21231 doesn't turn up anything relevant high in the results. So I'm at a dead end looking for the documentation.

馃敩 Minimal Reproduction

Attempt to load http://tsetse.info/must-type-assert-json-parse

馃敟 Exception or Error






Screen Shot 2020-12-23 at 10 58 17 AM

馃實 Your Environment

Operating System:

  
OSX
  

Output of bazel version:

  
bazel 3.7.2-homebrew
  

Rules_nodejs version:

(Please check that you have matching versions between WORKSPACE file and @bazel/* npm packages.)

  
3.0.0
  

Anything else relevant?

@alexeagle
Copy link
Collaborator

The docs are in https://github.com/bazelbuild/rules_nodejs/tree/stable/third_party/github.com/bazelbuild/rules_typescript/docs
But they need to be regenerated with the latest sources, and then figure out where to host it now that we slurped in rules_typescript.

Ideally tsetse should be a standalone project.

@alexeagle alexeagle self-assigned this Dec 23, 2020
@alexeagle
Copy link
Collaborator

Hmm it looks like I registered the tsetse.info domain under my alexeagle@google.com account which I no longer have. will take some research to figure out how to transfer it...

@mattem mattem added the question/docs Answer users question, preferably by updating docs label Jan 7, 2021
@EliSchleifer
Copy link

Maybe someone can add documentation to the rules_typescript/docs for this rule in the meantime. That folder hasn't been updated in 2 years.

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had any activity for 90 days. It will be closed if no further activity occurs in two weeks. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

@github-actions github-actions bot added the Can Close? We will close this in 30 days if there is no further activity label Apr 16, 2021
@mattem mattem added cleanup Tech debt, resolving it improves our own velocity and removed Can Close? We will close this in 30 days if there is no further activity labels Apr 16, 2021
@davido
Copy link

davido commented Feb 13, 2022

I also start seeing it after migration to 5.1.0:

  $ bazelisk build tools/node_tools/...
[...]
ERROR: /home/davido/projects/gerrit/tools/node_tools/polygerrit_app_preprocessor/BUILD:7:11: Compiling TypeScript (prodmode) //tools/node_tools/polygerrit_app_preprocessor:preprocessor failed: (Exit 1): tsc_wrapped.sh failed: error executing command (from target //tools/node_tools/polygerrit_app_preprocessor:preprocessor) bazel-out/k8-opt-exec-2B5CBBC6/bin/external/npm/@bazel/concatjs/bin/tsc_wrapped.sh @@bazel-out/k8-fastbuild/bin/tools/node_tools/polygerrit_app_preprocessor/preprocessor_tsconfig.json
tools/node_tools/polygerrit_app_preprocessor/links-updater.ts:42:40 - error TS21231: [tsetse] type assert `JSON.parse() as SomeExplicitType` for type & optimization safety.
	See http://tsetse.info/must-type-assert-json-parse.

42   const jsonRedirects: JSONRedirects = JSON.parse(fs.readFileSync(process.argv[3], {encoding: "utf-8"}));
                                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

ERROR: /home/davido/projects/gerrit/tools/node_tools/polygerrit_app_preprocessor/BUILD:7:11: Compiling TypeScript (devmode) //tools/node_tools/polygerrit_app_preprocessor:preprocessor failed: (Exit 1): tsc_wrapped.sh failed: error executing command (from target //tools/node_tools/polygerrit_app_preprocessor:preprocessor) bazel-out/k8-opt-exec-2B5CBBC6/bin/external/npm/@bazel/concatjs/bin/tsc_wrapped.sh @@bazel-out/k8-fastbuild/bin/tools/node_tools/polygerrit_app_preprocessor/preprocessor_es5_tsconfig.json
tools/node_tools/polygerrit_app_preprocessor/links-updater.ts:42:40 - error TS21231: [tsetse] type assert `JSON.parse() as SomeExplicitType` for type & optimization safety.
	See http://tsetse.info/must-type-assert-json-parse.

42   const jsonRedirects: JSONRedirects = JSON.parse(fs.readFileSync(process.argv[3], {encoding: "utf-8"}));
                                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

ERROR: /home/davido/projects/gerrit/tools/node_tools/polygerrit_app_preprocessor/BUILD:31:14 Bundling JavaScript tools/node_tools/polygerrit_app_preprocessor/links-updater-bundle.js [rollup] failed: (Exit 1): tsc_wrapped.sh failed: error executing command (from target //tools/node_tools/polygerrit_app_preprocessor:preprocessor) bazel-out/k8-opt-exec-2B5CBBC6/bin/external/npm/@bazel/concatjs/bin/tsc_wrapped.sh @@bazel-out/k8-fastbuild/bin/tools/node_tools/polygerrit_app_preprocessor/preprocessor_es5_tsconfig.json
INFO: Elapsed time: 52.872s, Critical Path: 47.48s
INFO: 61 processes: 54 internal, 3 linux-sandbox, 4 worker.
FAILED: Build did NOT complete successfully

This diff helped:

diff --git a/tools/node_tools/polygerrit_app_preprocessor/tsconfig.json b/tools/node_tools/polygerrit_app_preprocessor/tsconfig.json
index d3c7d1df13..384251b001 100644
--- a/tools/node_tools/polygerrit_app_preprocessor/tsconfig.json
+++ b/tools/node_tools/polygerrit_app_preprocessor/tsconfig.json
@@ -1,5 +1,13 @@
 {
   "compilerOptions": {
+    "plugins": [
+      {
+        "name": "@bazel/tsetse",
+        "disabledRules": [
+          "must-type-assert-json-parse"
+        ]
+      }
+    ],
     "target": "es2019", /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019' or 'ESNEXT'. */
     "module": "es2015", /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */
     "allowSyntheticDefaultImports": true,

@alexeagle
Copy link
Collaborator

I'd love to get tsetse up and running again, but I don't know of a path to get it to work with tsc and the integration with ts_library isn't going to be maintained anymore now that @bazel/concatjs package is in maintenance mode. I'm not going to have time but if anyone is familiar with the TS compiler apis and wants to help, that would be great.

@tylerhou
Copy link
Contributor

tylerhou commented Jul 5, 2022

(I was the original author of this rule. Sorry for the missing documentation; one possible reason the documentation is missing is because Google internally removed this rule in favor of changing the return type of JSON.parse to unknown.) Since this currently the top result for http://tsetse.info/must-type-assert-json-parse, I felt like I might as well document the rule here.

To fix this warning, do what the warning says: add a type assertion to the result of JSON.parse: JSON.parse(...) as MyType.

One rationale is because JSON.parse returns any, and generally any should be avoided because it introduces a potential hole in the type system. You can still use any, but you have to explicitly assert it as such JSON.parse(...) as any. Depending on your linter settings you might have to add the normal lintignore annotations for any.

The second rationale is that Closure will happily rename accesses on JSON.parse if there is no declared type for the result object. That is, Closure might mangle accesses on the obj, causing runtime errors that only show up when optimization is turned on (usually in production). For example, JSON.parse('{"hello": "world"}').hello might be compiled to JSON.parse('{"hello": "world"}').a when property-renamed. The fix is to create an interface for that type via declare interface Result { hello: string; }. The assertion isn't necessary鈥攐nce the interface is declared Closure won't rename the hello property when running under tsickle鈥攂ut enforcing the type assertion rule helped remind programmers to declare one. This rationale is mostly internal to Google; I don't think tsickle/Closure compiler usage is too common externally.

It was difficult to detect all cases like const obj: MyType = JSON.parse(...);, so I didn't bother. In those cases, you should rewrite the statement as const obj = JSON.parse(...) as MyType.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Tech debt, resolving it improves our own velocity help wanted question/docs Answer users question, preferably by updating docs
Projects
None yet
Development

No branches or pull requests

6 participants