test(path): add additional tests to normalize #5036
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
rwaskiewicz
commented
Nov 8, 2023
| @@ -249,6 +249,10 @@ export function join(...paths: string[]): string { | |||
| * @returns a resolved path! | |||
| */ | |||
| export function resolve(...paths: string[]): string { | |||
| /** | |||
There was a problem hiding this comment.
I have no tests/additional PRs for resolve - so I threw a very similar comment in here for completeness sake
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
force-pushed
the
rwaskiewicz/test/normalize-new
branch
from
23c7d0f to
0bc8f6a
Compare
|
| 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 | 417 |
| TS2322 | 394 |
| 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 | 251 | resolve |
| src/utils/index.ts | 269 | 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 8, 2023
|
oh still draft lol 😅 |
|
@alicewriteswrongs Lol - it's done, I was just waiting for CI to pass just in case I missed something when I pulled it out of the larger |
tanner-reits
approved these changes
Nov 8, 2023
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 <[email protected]>
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 <[email protected]>
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 how this 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.
GitHub Issue Number: N/A
What is the new behavior?
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:Does this introduce a breaking change?
Testing
No functional changes - added unit tests only
Other information
related to: #5029, #5032