Fix “noImplicitAny” errors in “x-template.js”.#360
Conversation
| @@ -1,3 +1,3 @@ | |||
| export const render: (container: HTMLElement, rawResult: unknown) => void; | |||
| export const html: (strings: string[], ...values: unknown[]) => unknown; | |||
| export const html: (strings: TemplateStringsArray, ...values: unknown[]) => unknown; | |||
There was a problem hiding this comment.
It probably always should have been this. The only meaningful change is that .raw should be part of this interface. Should be no issue with the current eslint-plugin-x-element module usage — not that that even uses TS.
| @@ -1,7 +1,55 @@ | |||
| // TODO: #357: Replace remaining `any` with precise types or `unknown`. | |||
| /* eslint-disable jsdoc/reject-any-type */ | |||
There was a problem hiding this comment.
This is a much more manageable opt-out. Still file-level, but I am alright with accepting some any usage while we fry the bigger fish.
The goal here is to try and accept TypeScript defaults first. Then, we can move on to making this linter happy later. In the meantime, I cannot accept adding runtime bloat — and using any is allowing us to avoid dealing with runtime changes.
| const childNode = TemplateEngine.#document.createElement(tagName); | ||
| state.tagName === 'template' | ||
| // @ts-ignore — TypeScript doesn’t get that this is a template. | ||
| // @ts-expect-error — TS doesn’t get that this is a template. |
There was a problem hiding this comment.
I flipped these all to @ts-expect-error — there’s no real reason we should have been using @ts-ignore, it was just an oversight. The benefit of @ts-expect-error is that it will yell at you as soon as the error goes away. I.e., it tells you if / when it can actually just be deleted.
theengineear
force-pushed
the
fix-no-implicit-any-in-template
branch
from
aec8bb4 to
3b48eac
Compare
| // Parser guarantees these arrays are non-empty at this point. | ||
| state.element = /** @type {DocumentFragment | Element} */ (state.parentElements.pop()); | ||
| state.tagName = /** @type {TagName} */ (state.parentTagNames.pop()); | ||
| state.childNodesIndex = /** @type {number} */ (state.path.pop()); |
There was a problem hiding this comment.
☝️ — Rather than let a weaker typing cascade around, these just get cast. We technically know these will be correctly defined, even if TS doesn’t. We may be able to clean up later, but it would be out of scope to make more runtime changes.
| // @ts-expect-error — TS doesn’t get that this is an element. | ||
| state.element.setAttribute(state.name, decoded); | ||
| state.name = null; | ||
| state.name = ''; |
There was a problem hiding this comment.
RUNTIME CHANGE! We were just using null to reset this value. It makes things simpler to reset it with '' instead.
| onText(state.path); | ||
| break; | ||
| case XParser.tokenTypes.boundContentValue: | ||
| // @ts-ignore — TypeScript doesn’t get that this is an element. |
| if (previousSibling !== startNode) { | ||
| TemplateEngine.#removeBetween(startNode, node); | ||
| } | ||
| // @ts-expect-error — TS doesn’t know parentNode is non-null. |
There was a problem hiding this comment.
Again, to be clear. We could type-narrow in the runtime here… but that feels overly-defensive. And, it wouldn’t fool our code-coverage, which would start complaining if we had unreachable forks. I am totally happy with a robust test suite (at 100% coverage) and a handful of these @ts-expect-error lines for now.
|
@klebba — another round of cleanup. Again… I am being absurdly conservative about making runtime changes. There is really only one conceptual runtime change in here… and that’s to change the default / reset value for |
theengineear
force-pushed
the
fix-no-implicit-any-in-template
branch
from
3b48eac to
5377bba
Compare
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. Note, we _did_ have to disable “jsdoc/reject-any-type” for this file temporarily. Ideally, we can leverage `unknown` types versus `any` types in the future, that that is a larger refactor which would require much more scrutiny and bloat the scope of work.
theengineear
force-pushed
the
fix-no-implicit-any-in-template
branch
from
5377bba to
9e5137a
Compare
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.
Note, we did have to disable “jsdoc/reject-any-type” for this file temporarily. Ideally, we can leverage
unknowntypes versusanytypes in the future, that that is a larger refactor which would require much more scrutiny and bloat the scope of work.