-
Notifications
You must be signed in to change notification settings - Fork 789
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
fix(compiler): normalize paths on windows #4997
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/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 | 416 |
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 14 unused exports on this PR. That's 1 fewer than on main
! 🎉🎉🎉
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 | 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 | 61 | satisfies |
src/compiler/types/validate-primary-package-output-target.ts | 61 | Record |
src/testing/puppeteer/puppeteer-declarations.ts | 485 | WaitForEventOptions |
src/compiler/sys/fetch/write-fetch-success.ts | 7 | writeFetchSuccessSync |
b5cd087
to
ae97e40
Compare
a4c3950
to
08d30c2
Compare
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
this commit removes a spy on `join.resolve` and updates the mocks to properly spy on the wrapped `@utils`' `resolve` fn
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
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).
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
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).
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
08d30c2
to
a7c6b49
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good. Tested out the steps listed in the PR body and things appear to be working correctly (not on a Windows machine, so can't verify that aspect).
What is the current behavior?
See 'New Behavior' section
What is the new behavior?
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:normalizePath
function to convert Windows-style path separators (
\\
) toUnix/POSIX-style path separators (
/
)path
implementations for each ofthese functions.
as a result, calling
join
or any of the other three methods, even whenimporting the method from
path
like below would result in the polyfillbeing called:
while this was 'nice' in that stencil engineers didn't need to think
about which implementation of
path
functions they were using, thispolyfill 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:
discrepencies arose where parts of the code would explicitly wrap a
call to
join()
(or one of its ilk) around a path normalizationfunction. 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:
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.Does this introduce a breaking change?
Testing
A dev build of Stencil containing the changes in this commit was built, and is available under
@stencil/[email protected]
Reproduction cases for the issues that this pull requests closes have been combined into a single repository/reproduction case. This reproduction project can be found here. It uses the aforementioned dev build of Stencil.
The following sections assume that the reproduction repo linked above has been cloned locally and dependencies installed (
npm ci
). Note that manual testing should occur on a Windows based machine.Copy Tasks
GitHub Issue: #4980
npm start
src/dev/worksonmymachine.html
have been copied towww/dev/worksonmymachine.html
(open the both to confirm that they match).src/dev/worksonmymachine.html
- add text, write a blog, add non-sense - whatever feels right. We only need one character of difference here.www/dev/worksonmymachine.html
, observe your changes made tosrc/dev/worksonmymachine.html
have been propagated to the copied file underwww/dev/
Global Styles
GitHub Issue: #4961
npm start
src/global.css
. This file contains the CSS variable that is currently set to blue.red
,green
, etc.)CacheDir
The cache directory setting (
cacheDir
) is now unconditionally normalized.Previously, absolute paths would not get transformed.
To evaluate this, update the project's
stencil.config.ts
as follows:Running the build
npm run build
should result in cached files being placed in a.stencil
directory in this project's root.Test Paths
Some test cases associated with
validate-testing
were updated in the associated pull request/dev build. These changes are believed to be correct/validated by runningnpm t
in the reproduction repository (and are equally exercised by Stencil's CI) - specifically, these ensure that the Jest infrastructure is found on disk and loaded properly. If it weren't tests would fall over/fail!Ionic Framework
For additional testing, a one-off build of the Ionic Framework "Stencil Nightly Build" with the aforementioned dev build related to this PR was run and passed
Other information
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