chore(utils): handle a few SNC violations related to path extensions and diagnostics#4903
chore(utils): handle a few SNC violations related to path extensions and diagnostics#4903tanner-reits merged 3 commits intomainfrom
Conversation
|
| 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 | 429 |
| TS2322 | 406 |
| TS18048 | 310 |
| TS18047 | 100 |
| TS2722 | 38 |
| TS2532 | 34 |
| 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 | 123 | 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 |
| const dups = new Set<string>(); | ||
|
|
||
| for (let i = 0; i < diagnostics.length; i++) { | ||
| const d = normalizeDiagnostic(compilerCtx, diagnostics[i]); |
There was a problem hiding this comment.
Renamed this since we import our declaration types as d which just made it confusing
| lineNumber: beforeLineIndex + 1, | ||
| text: srcLines[beforeLineIndex], | ||
| errorCharStart: -1, | ||
| if (diagnostic.absFilePath) { |
There was a problem hiding this comment.
Seems like a big diff, just moved all the existing code under this check
| const filePath = normalizePath(pathParts[0]); | ||
| const ext = filePath.split('.').pop().toLowerCase(); | ||
| const filePathParts = filePath.split('.'); | ||
| const ext = filePathParts.length > 1 ? filePathParts.pop()!.toLowerCase() : null; |
There was a problem hiding this comment.
Felt wrong to return the path as the "extension" if it did not actually have an extension, so allowed this to have a null value. That's why the associated test had to change here too
There was a problem hiding this comment.
are we pretty certain nothing depends on that old behavior?
There was a problem hiding this comment.
The only thing I could think of is dot files (e.g. .eslintrc) - but I'm not entirely sure/will leave that to @tanner-reits 😉
There was a problem hiding this comment.
I don't think this change will mess with anything. This function is only used in a single Rollup plugin where we only use the extension for two cases:
- If the extension is in a pre-defined list of extensions. Which, as of now, only includes
txt,frag, andvert. Truthfully, I have no idea what a.fragor.vertfile are 🤷♂️ - Another check for if it's an
svg
In all these cases. Since there is a finite list of traditional extensions, we can safely change the function to return null if there isn't an extension and know we will just pass over these checks. As for dot-files (even though they don't really matter here), we'd still return the filename as the "extension" as before.
| const getExt = (filePath: string): string | null => { | ||
| const fileParts = filePath.split('.'); | ||
|
|
||
| return fileParts.length > 1 ? fileParts.pop()!.toLowerCase() : null; |
There was a problem hiding this comment.
No action required - I think we can avoid having to use the bang operator if we break this down into 2 statements:
| return fileParts.length > 1 ? fileParts.pop()!.toLowerCase() : null; | |
| const extension = fileParts.pop(); | |
| return extension ? extension.toLowerCase() : null; |
That said, I doubt the typings for Array.pop will change such that the bang operator we have here today will be unnecessary, so no action required
| const filePath = normalizePath(pathParts[0]); | ||
| const ext = filePath.split('.').pop().toLowerCase(); | ||
| const filePathParts = filePath.split('.'); | ||
| const ext = filePathParts.length > 1 ? filePathParts.pop()!.toLowerCase() : null; |
There was a problem hiding this comment.
are we pretty certain nothing depends on that old behavior?
What is the current behavior?
SNC violations are bad
GitHub Issue Number: N/A
What is the new behavior?
There are less of them 🎉
Does this introduce a breaking change?
Testing
Tests continue to pass
Other information