fix(config): improved warning when root path includes bad characters #15761
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
Previously a warning was only issued when the root path included the `#` character, but the `?` character is equally bad.
|
|
poppa
changed the title
9 tasks
This is pretty much a dummy commit to get the CI checks to run again. The previous one failed, I would say, intermittently and unrelated to any code changes.
|
I can't see that the failing test on Windows has anything to do with this PR? |
Build&Test broke on Windows with the following error: ``` jq: error: syntax error, unexpected INVALID_CHARACTER (Windows cmd shell quoting issues?) at <top-level>, line 1: .[0].devDependencies[\"playwright-chromium\"].version ``` Seems strange this would break now since the escaped quotes had been there for at least 14 months.
Well, that was something unrelated, which I seem to have fixed. |
|
So, how does this error happen? There seems to be some intermittent errors occuring in the test suite overall. I get this intermittently when running the test suite locally: |
bluwy
reviewed
Feb 6, 2024
bluwy
previously approved these changes
Feb 7, 2024
| $env:PLAYWRIGHT_VERSION="$(pnpm ls --depth 0 --json -w playwright-chromium | jq --raw-output '.[0].devDependencies[\"playwright-chromium\"].version')" | ||
| $env:PLAYWRIGHT_VERSION="$(pnpm ls --depth 0 --json -w playwright-chromium | jq --raw-output '.[0].devDependencies["playwright-chromium"].version')" |
There was a problem hiding this comment.
Thanks for fixing this! The issue has been haunting us in CI for many weeks now.
patak-dev
reviewed
Feb 7, 2024
| const logger = createLogger('info') | ||
| logger.warn = (str) => { | ||
| expect(normalizeString(str)).toEqual( | ||
| 'The project root contains the "?" character (/inc?udes), which may ' + |
There was a problem hiding this comment.
I think we may be introducing to many, too specific, tests here for this feature. I would be ok with a single one and to have a includes instead of equal test so we avoid having to maintain all this in sync if we later change the wording.
There was a problem hiding this comment.
Agree 👍 I moved this as a unit test with only a single.
patak-dev
approved these changes
Feb 7, 2024
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.
Previously a warning was only issued when the root path included the
#character, but the?character is equally bad.Description
This should fix #15726.
Root paths can not include the
#nor the?character. ESM is using URLs to resolve imports, and in an URL what follows those characters is not part of the path.Additional context
I saw there's another PR for this (#15755), but this PR is a bit more elaborative and includes some tests as well.
What is the purpose of this pull request?
I would consider it an overall improvement.
Before submitting the PR, please make sure you do the following
fixes #123).