-
Notifications
You must be signed in to change notification settings - Fork 787
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test(path): add additional tests to relative #5029
Merged
Merged
Conversation
This file contains 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
add additional tests to stencil's wrapped version of `path.relative`, adding additional documentation to how the wrapped function work. specifically, add tests to verify the following behavior: 1. when there is a single directory of overlap between `from` and `to` 2. when there is multiple directory overlap, but no divergences between `from` and `to` 3. when there is complete overlap between `from` and `to` the latter case of complete overlap is documented in JSDoc as well to signify a divergence from `path.resolve`. this behavior was the impetus for this change - I didn't understand what I was seeing at first when debugging some Stencil 4.x related path related bugs. this commit is spun out from that work
|
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/client-hydrate.ts | 19 |
src/screenshot/connector-base.ts | 19 |
src/runtime/vdom/vdom-render.ts | 18 |
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 | 418 |
TS2322 | 395 |
TS18048 | 310 |
TS18047 | 101 |
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 |
TS2430 | 1 |
Unused exports report
There are 15 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 | 145 | CUSTOM |
src/utils/index.ts | 241 | resolve |
src/utils/index.ts | 255 | normalize |
src/utils/index.ts | 7 | escapeRegExpSpecialCharacters |
src/compiler/app-core/app-data.ts | 25 | BUILD |
src/compiler/app-core/app-data.ts | 115 | Env |
src/compiler/app-core/app-data.ts | 117 | 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 |
alicewriteswrongs
approved these changes
Nov 7, 2023
rwaskiewicz
added a commit
that referenced
this pull request
Nov 8, 2023
add additional tests to stencil's wrapped version of `path.join`, adding additional documentation to how the wrapped function works. specifically, add tests to verify the following behavior: 1. how trailing slashes are treated by the function 2. how 1+ string arguments of '/' are handled 3. how relative paths going 'up' one directory level are handled such that only a file is resolved the first case of trailing slashes is documented in JSDoc as well to signify a divergence from `path.joing`. this behavior was the impetus for this change - I didn't understand what I was seeing at first when debugging some Stencil 4.x related path related bugs. this commit is spun out from that work. related to: #5029
2 tasks
tanner-reits
approved these changes
Nov 8, 2023
rwaskiewicz
added a commit
that referenced
this pull request
Nov 8, 2023
add additional tests to stencil's wrapped version of `path.join`, adding additional documentation to how the wrapped function works. specifically, add tests to verify the following behavior: 1. how trailing slashes are treated by the function 2. how 1+ string arguments of '/' are handled 3. how relative paths going 'up' one directory level are handled such that only a file is resolved the first case of trailing slashes is documented in JSDoc as well to signify a divergence from `path.joing`. this behavior was the impetus for this change - I didn't understand what I was seeing at first when debugging some Stencil 4.x related path related bugs. this commit is spun out from that work. related to: #5029
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 8, 2023
add additional tests to stencil's wrapped version of `path.join`, adding additional documentation to how the wrapped function works. specifically, add tests to verify the following behavior: 1. how trailing slashes are treated by the function 2. how 1+ string arguments of '/' are handled 3. how relative paths going 'up' one directory level are handled such that only a file is resolved the first case of trailing slashes is documented in JSDoc as well to signify a divergence from `path.joing`. this behavior was the impetus for this change - I didn't understand what I was seeing at first when debugging some Stencil 4.x related path related bugs. this commit is spun out from that work. related to: #5029
rwaskiewicz
added a commit
that referenced
this pull request
Nov 8, 2023
add additional tests to stencil's wrapped version of `path.normalize`, adding additional documentation to how the wrapped function works. specifically, add tests to verify the following behavior: 1. how an empty string behaves 2. how relative paths ('.', '..') behave 3. how absolute paths ('/' for POSIX and '\\' for Windows) behave questioning how thise function behaves/should behave was the impetus for this change - I didn't understand what I was seeing at first when debugging some Stencil 4.x related path related bugs. this commit is spun out from that work. related to: #5029, #5032
2 tasks
rwaskiewicz
added a commit
that referenced
this pull request
Nov 8, 2023
add additional tests to stencil's wrapped version of `path.normalize`, adding additional documentation to how the wrapped function works. specifically, add tests to verify the following behavior: 1. how an empty string behaves 2. how relative paths ('.', '..') behave 3. how absolute paths ('/' for POSIX and '\\' for Windows) behave questioning how thise function behaves/should behave was the impetus for this change - I didn't understand what I was seeing at first when debugging some Stencil 4.x related path related bugs. this commit is spun out from that work. related to: #5029, #5032
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 9, 2023
add additional tests to stencil's wrapped version of `path.normalize`, adding additional documentation to how the wrapped function works. specifically, add tests to verify the following behavior: 1. how an empty string behaves 2. how relative paths ('.', '..') behave 3. how absolute paths ('/' for POSIX and '\\' for Windows) behave questioning how thise function behaves/should behave was the impetus for this change - I didn't understand what I was seeing at first when debugging some Stencil 4.x related path related bugs. this commit is spun out from that work. related to: #5029, #5032
rwaskiewicz
added a commit
that referenced
this pull request
Nov 9, 2023
this patch ensures that paths are properly normalized by the compiler. this ensures that regardless of the platform (operating system) that a project is compiled on, paths are uniformly treated internally by stencil. this has system-wide reaching effects - from the in-memory filesystem, to configuration/output target validation, and file generation. previously, stencil's in-browser compilation support included a polyfill for the following NodeJS `path` module functions: `join`, `normalize`, `relative` & `resolve`. this polyfill did the following: - it wrapped each of the aforementioned functions in a `normalizePath` function to convert Windows-style path separators (`\\`) to Unix/POSIX-style path separators (`/`) - it overwrote the standard NodeJS `path` implementations for each of these functions. as a result, calling `join` or any of the other three methods, even when importing the method from `path` like below would result in the polyfill being called: ```ts import { join } from 'path'; // this imports the polyfilled `join` // runs the native `path.join`, then normalizes the returned path const filePath = join(part1, part2); ``` while this was 'nice' in that stencil engineers didn't need to think about which implementation of `path` functions they were using, this polyfill made some behavior of the compiler hard to understand. the polyfills were removed in #4317 (b042d8b). this led to calls to the aforementioned functions to call their original implementations, rather than the wrapped implementations: ```ts import { join } from 'path'; // imports Node's `join` // run the native `path.join`, without any normalization const filePath = join(part1, part2); ``` discrepencies arose where parts of the code would explicitly wrap a call to `join()` (or one of its ilk) around a path normalization function. this caused paths to not be uniformly normalized throughout the codebase, leading to errors. since the removal of in-browser compilation, additional pull requests to fix path-related issues on windows have landed in the codebase: - #4545 (cd58d9c) - #4932 (b97dadc) this commit builds on the previous commits by attempting to move stencil's compiler completely over to polyfilled versions of the mentioned `path` functions that are explicitly imported/called. Some of the changes found herein were created/validated using Alice's codemod branch, #4996 Co-authored-by: alicewriteswrongs <alicewriteswrongs@users.noreply.github.com> STENCIL-975 Determine Scope of Path Polyfill Bug, Fix Fixes: #4980 Fixes: #4961 Spun Off: #5036, #5032, #5029
2 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 10, 2023
* fix(compiler): normalize paths on windows this patch ensures that paths are properly normalized by the compiler. this ensures that regardless of the platform (operating system) that a project is compiled on, paths are uniformly treated internally by stencil. this has system-wide reaching effects - from the in-memory filesystem, to configuration/output target validation, and file generation. previously, stencil's in-browser compilation support included a polyfill for the following NodeJS `path` module functions: `join`, `normalize`, `relative` & `resolve`. this polyfill did the following: - it wrapped each of the aforementioned functions in a `normalizePath` function to convert Windows-style path separators (`\\`) to Unix/POSIX-style path separators (`/`) - it overwrote the standard NodeJS `path` implementations for each of these functions. as a result, calling `join` or any of the other three methods, even when importing the method from `path` like below would result in the polyfill being called: ```ts import { join } from 'path'; // this imports the polyfilled `join` // runs the native `path.join`, then normalizes the returned path const filePath = join(part1, part2); ``` while this was 'nice' in that stencil engineers didn't need to think about which implementation of `path` functions they were using, this polyfill made some behavior of the compiler hard to understand. the polyfills were removed in #4317 (b042d8b). this led to calls to the aforementioned functions to call their original implementations, rather than the wrapped implementations: ```ts import { join } from 'path'; // imports Node's `join` // run the native `path.join`, without any normalization const filePath = join(part1, part2); ``` discrepencies arose where parts of the code would explicitly wrap a call to `join()` (or one of its ilk) around a path normalization function. this caused paths to not be uniformly normalized throughout the codebase, leading to errors. since the removal of in-browser compilation, additional pull requests to fix path-related issues on windows have landed in the codebase: - #4545 (cd58d9c) - #4932 (b97dadc) this commit builds on the previous commits by attempting to move stencil's compiler completely over to polyfilled versions of the mentioned `path` functions that are explicitly imported/called. Some of the changes found herein were created/validated using Alice's codemod branch, #4996 Co-authored-by: alicewriteswrongs <alicewriteswrongs@users.noreply.github.com> STENCIL-975 Determine Scope of Path Polyfill Bug, Fix Fixes: #4980 Fixes: #4961 Spun Off: #5036, #5032, #5029 * fix validate-dist-collection tests * fix prerendered-write-path test * fix stencil-types tests this commit removes a spy on `join.resolve` and updates the mocks to properly spy on the wrapped `@utils`' `resolve` fn * fix validate-paths tests this commit updates the tests for `validate-paths`. it replaces instances of `path.join` in assertions with stencil's own `join` function. existing instances of `path.join` have been left as-is if they pertain to user input. this allows us to exercise a user using either path separator in CI (which runs in windows and linux). the cache directory is now normalized if it is absolute. otherwise, provided cache directories in `stencil.config.ts` would never get normalized * fix validate-output-www tests this commit updates the tests for `validate-paths`. it replaces instances of `path.join` in assertions with stencil's own `join` function. existing instances of `path.join` have been left as-is if they pertain to user input. this allows us to exercise a user using either path separator in CI (which runs in windows and linux). * fix validate-output-dist tests this commit updates the tests for `validate-output`. it replaces instances of `path.join` in assertions with stencil's own `join` function. existing instances of `path.join` have been left as-is if they pertain to user input. this allows us to exercise a user using either path separator in CI (which runs in windows and linux). this commit also switches `output-target.ts#getcomponentsDtsSrcFilePath` over to use Stencil's join function to fix the tests * fix validate-output-dist-custom-element tests this commit updates the tests for `validate-output-dist-custom-element`. it replaces instances of `path.join` in assertions with stencil's own `join function. existing instances of `path.join` have been left as-is if they pertain to user input. this allows us to exercise a user using either path separator in CI (which runs in windows and linux). * fix validate-testing tests this commit updates the tests for `validate-testing`. it replaces instances of `path.join` in assertions with stencil's own `join` function. existing instances of `path.join` have been left as-is if they pertain to user input. this allows us to exercise a user using either path separator in CI (which runs in windows and linux). the screenshot connector is now normalized if it is absolute. otherwise, provided connector file path in `stencil.config.ts` would never get normalized
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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?
I was questioning some divergent behavior between our
path.resolve
wrapper and the underlying Node call. Specifically:GitHub Issue Number: N/A
What is the new behavior?
add additional tests to stencil's wrapped version of
path.relative
, adding additional documentation to how the wrapped function work. specifically, add tests to verify the following behavior:from
andto
from
andto
from
andto
the latter case of complete overlap is documented in JSDoc as well to signify a divergence from
path.resolve
.Does this introduce a breaking change?
Testing
No functional changes have occurred - only adding unit tests. I did track the execution of the tests in a debugger and walked through
normalizePath
to verify they were hitting the code points that I expected them to.Other information
the case where we have complete overlap between
from
andto
was the impetus for this change - I didn't understand what I was seeing at first when debugging some Stencil 4.x related path related bugs. this commit is spun out from that work