Fix “noImplicitAny” errors in “x-parser.js”.#359
Conversation
With the change from TypeScript “5.x” to “6.x”, the “strict” config setting now defaults to “true”. We are simply working through the files one-by-one to be able to accept that new default change. It’s ultimately what we want, anyhow.
| * parsing errors are allotted numbers #100-#199. | ||
| * @param {string | undefined} key | ||
| * @returns {string | undefined} | ||
| */ |
There was a problem hiding this comment.
☝️ — This was 99.9% just straight annotations. It turns out that this file was pretty well-suited for stricter typing… perhaps not a surprise.
| let stringLength = 0; | ||
| let stringIndex = 0; | ||
| let nextStringIndex = 0; | ||
| let hasStringContext = false; |
There was a problem hiding this comment.
☝️ — Here is the bulk of the runtime “changes”. Rather than initializing to null… we simply initialize to a sensible, typed default. I.e., like '' or 0.
Note that we add a single flag called hasStringContext which we use in the error fork only. It allows us to not break our public contract with tools like eslint-plugin-x-element which look to see if start / end are specifically null — which indicates that no string context is available and a diagnostic needs to be printed at a more generic text span in an IDE (yes, we had a test covering that 😉).
| break; | ||
| case XParser.#startTagClose: | ||
| if (XParser.#voidHtmlElements.has(tagName)) { | ||
| if (tagName !== null && XParser.#voidHtmlElements.has(tagName)) { |
There was a problem hiding this comment.
This runtime null check would be ugly to try and avoid since null cannot be a key for a string set. This single, strict null check only happens on a start-tag-close. I am not concerned that this is a performance issue and I cannot detect any sort of uptick from our performance tests.
| break; | ||
| } | ||
| } | ||
| // Cast: nextStringIndex is guaranteed non-null here — it’s always |
There was a problem hiding this comment.
This was a pleasant surprise. Because we now return never from our error functions (and because we initialize variables with types), TypeScript can fully infer this. Hooray.
| const start = stringIndex; | ||
| const end = nextStringIndex; | ||
| const start = hasStringContext ? stringIndex : null; | ||
| const end = hasStringContext ? nextStringIndex : null; |
There was a problem hiding this comment.
☝️ — Again, we need to specifically set start / end to null if we don’t actually know where we are in the string. This is important for consumers looking to create text span diagnostics.
|
FYI @klebba — working through these changes. Just getting a little better about annotating our internal types. So far, this is pretty straightforward as it’s just more commentary. I think we have tended to write our code with very strict, positional input parameters — so this isn’t much of a lift. If we ever needed to do something drastic in the runtime, I would hesitate to do this stuff — but that’s not the pattern I’m seeing (so far). |
| static #validTransition(string: any, stringIndex: any, value: any): any; | ||
| static #getErrorInfo(strings: any, stringsIndex: any, string: any, stringIndex: any): { | ||
| parsed: any; | ||
| /** |
There was a problem hiding this comment.
Ideally, all this stuff would not end up in the emitted type declarations… but… I am hesitant to manually strip it (since it feels mostly harmless). I did leave commentary on microsoft/TypeScript#58145 about this. Maybe we can get TS to strip this for us one day.
1 participant
With the change from TypeScript “5.x” to “6.x”, the “strict” config setting now defaults to “true”. We are simply working through the files one-by-one to be able to accept that new default change. It’s ultimately what we want, anyhow.