chore(snc): fix violations in update-element.spec#4905
Merged
rwaskiewicz merged 3 commits intomainfrom Oct 9, 2023
Merged
Conversation
this commit widens the types of multiple VNode variable instances to `VNode | null` from `VNode`. in each of thsee usages, an old VNode instance is instantiated as `null` for testing purposes to call `updateElement`. since the old VNode can be `null` according to the `udpateElement` function signature, this is considered to by a safe operation (i.e. does not adversely affect the tests).
the test that initializes these VNodes doesn't use these values stored on the VNode, making this a safe operation in terms of the test
Contributor
|
| Path | Error Count |
|---|---|
| src/dev-server/index.ts | 37 |
| src/mock-doc/serialize-node.ts | 36 |
| src/dev-server/server-process.ts | 32 |
| src/compiler/build/build-stats.ts | 27 |
| src/compiler/output-targets/dist-lazy/generate-lazy-module.ts | 25 |
| src/compiler/style/test/optimize-css.spec.ts | 23 |
| src/testing/puppeteer/puppeteer-element.ts | 23 |
| src/compiler/prerender/prerender-main.ts | 22 |
| src/runtime/vdom/vdom-render.ts | 20 |
| src/runtime/client-hydrate.ts | 19 |
| src/screenshot/connector-base.ts | 19 |
| src/compiler/config/test/validate-paths.spec.ts | 16 |
| src/dev-server/request-handler.ts | 15 |
| src/compiler/prerender/prerender-optimize.ts | 14 |
| src/compiler/sys/stencil-sys.ts | 14 |
| src/compiler/transpile/transpile-module.ts | 14 |
| src/runtime/vdom/vdom-annotations.ts | 14 |
| src/sys/node/node-sys.ts | 14 |
| src/compiler/build/build-finish.ts | 13 |
| src/compiler/prerender/prerender-queue.ts | 13 |
Our most common errors
| Typescript Error Code | Count |
|---|---|
| TS2345 | 428 |
| TS2322 | 400 |
| TS18048 | 312 |
| TS18047 | 100 |
| TS2722 | 38 |
| TS2532 | 36 |
| TS2531 | 23 |
| TS2454 | 14 |
| TS2352 | 13 |
| TS2769 | 10 |
| TS2790 | 10 |
| TS2538 | 8 |
| TS2344 | 5 |
| TS2416 | 4 |
| TS2493 | 3 |
| TS18046 | 2 |
| TS2684 | 1 |
| TS2488 | 1 |
| TS2464 | 1 |
| TS2430 | 1 |
Unused exports report
There are 12 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!
Unused exports
| File | Line | Identifier |
|---|---|---|
| src/runtime/bootstrap-lazy.ts | 21 | setNonce |
| src/screenshot/screenshot-fs.ts | 18 | readScreenshotData |
| src/testing/testing-utils.ts | 198 | withSilentWarn |
| src/utils/index.ts | 140 | CUSTOM |
| src/compiler/app-core/app-data.ts | 25 | BUILD |
| src/compiler/app-core/app-data.ts | 114 | Env |
| src/compiler/app-core/app-data.ts | 116 | NAMESPACE |
| src/compiler/fs-watch/fs-watch-rebuild.ts | 110 | updateCacheFromRebuild |
| src/compiler/types/validate-primary-package-output-target.ts | 62 | satisfies |
| src/compiler/types/validate-primary-package-output-target.ts | 62 | Record |
| src/testing/puppeteer/puppeteer-declarations.ts | 485 | WaitForEventOptions |
| src/compiler/sys/fetch/write-fetch-success.ts | 7 | writeFetchSuccessSync |
| it('should add new class when no oldVNode', () => { | ||
| const elm = document.createElement('my-tag') as HTMLElement; | ||
| const oldVNode: d.VNode = null; | ||
| const oldVNode: d.VNode | null = null; |
Contributor
There was a problem hiding this comment.
could we just omit the annotation for these instead? I think that should be ok since the type of oldVNode for updateElement is already d.VNode | null
Contributor
There was a problem hiding this comment.
(or equivalently just type them as null)
Member
Author
There was a problem hiding this comment.
Yeah, I think we'd have to do the latter (type them as null), since they'd implicitly have any type (causing the compiler to yell at us) - done in 1cc54b7
alicewriteswrongs
approved these changes
Oct 6, 2023
tanner-reits
approved these changes
Oct 9, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the current behavior?
We have quite a few SNCs! This is today's 15 minutes of work to drive these down
GitHub Issue Number: N/A
What is the new behavior?
this commit updates the types of multiple VNode variable instances to
nullfromVNode. in each of these usages, an old VNodeinstance is instantiated as
nullfor testing purposes to callupdateElement. since the old VNode can benullaccording to theupdateElementfunction signature, this is considered to by a safeoperation (i.e. does not adversely affect the tests).
for one test in particular, this commit updates VNode init by providing
the VNode with non-null values. the test that initializes these VNodes
doesn't use these values stored on the VNode, making this a safe
operation in terms of the test
Does this introduce a breaking change?
Testing
Unit tests continue to pass - reading through the code and tests, I don't have any reason to believe we've changed their actual behavior in the second commit (the first acts solely on the type system)