fix(config): improved warning when root path includes bad characters by poppa · Pull Request #15761 · vitejs/vite

poppa · 2024-01-31T09:32:56Z

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?

Other

I would consider it an overall improvement.

Before submitting the PR, please make sure you do the following

Read the Contributing Guidelines, especially the Pull Request Guidelines.
Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
Ideally, include relevant tests that fail without this PR but pass with it.

Previously a warning was only issued when the root path included the `#` character, but the `?` character is equally bad.

bolt-new-by-stackblitz · 2024-01-31T09:32:59Z

Run & review this pull request in StackBlitz Codeflow.

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.

poppa · 2024-02-01T06:12:19Z

I can't see that the failing test on Windows has anything to do with this PR?

Run echo "PLAYWRIGHT_BROWSERS_PATH=$HOME\.cache\playwright-bin" >> $env:GITHUB_ENV
  echo "PLAYWRIGHT_BROWSERS_PATH=$HOME\.cache\playwright-bin" >> $env:GITHUB_ENV
  $env:PLAYWRIGHT_VERSION="$(pnpm ls --depth 0 --json -w playwright-chromium | jq --raw-output '.[0].devDependencies[\"playwright-chromium\"].version')"
  echo "PLAYWRIGHT_VERSION=$env:PLAYWRIGHT_VERSION" >> $env:GITHUB_ENV
  shell: C:\Program Files\PowerShell\7\pwsh.EXE -command ". '{0}'"
  env:
    NODE_OPTIONS: --max-old-space-size=6144
    PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD: 1
    VITEST_SEGFAULT_RETRY: 3
    PNPM_HOME: C:\Users\runneradmin\setup-pnpm\node_modules\.bin
jq: error: syntax error, unexpected INVALID_CHARACTER (Windows cmd shell quoting issues?) at <top-level>, line 1:
.[0].devDependencies[\"playwright-chromium\"].version                     
jq: 1 compile error
ResourceUnavailable: D:\a\_temp\af6fc96b-044b-47f4-a84e-a29f7ea013f2.ps1:3
Line |
   3 |  … _VERSION="$(pnpm ls --depth 0 --json -w playwright-chromium | jq --ra …
     |                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Program 'pnpm.CMD' failed to run: The pipe is being closed.At
     | D:\a\_temp\af6fc96b-044b-47f4-a84e-a29f7ea013f2.ps1:3 char:28 + … HT_VERSION="$(pnpm ls --depth 0 --json -w
     | playwright-chromium | jq -- … +                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~.
Error: Process completed with exit code 1.

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.

poppa · 2024-02-01T17:31:49Z

I can't see that the failing test on Windows has anything to do with this PR?

[...]

Well, that was something unrelated, which I seem to have fixed.

poppa · 2024-02-01T17:42:35Z

So, how does this error happen?

"WebSocket server error: Port is already in use"

There seems to be some intermittent errors occuring in the test suite overall.

I get this intermittently when running the test suite locally:

 FAIL  playground/fs-serve/__tests__/base/fs-serve-base.spec.ts > main > safe fetch with query
AssertionError: expected '' to include 'KEY=safe'
 ❯ playground/fs-serve/__tests__/base/fs-serve-base.spec.ts:28:57

bluwy · 2024-02-07T08:25:49Z

        run: |
          echo "PLAYWRIGHT_BROWSERS_PATH=$HOME\.cache\playwright-bin" >> $env:GITHUB_ENV
-          $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')"


Thanks for fixing this! The issue has been haunting us in CI for many weeks now.

patak-cat

Oh, nice!

patak-cat · 2024-02-07T09:04:15Z

+  const logger = createLogger('info')
+  logger.warn = (str) => {
+    expect(normalizeString(str)).toEqual(
+      'The project root contains the "?" character (/inc?udes), which may ' +


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.

Agree 👍 I moved this as a unit test with only a single.

patak-cat · 2024-02-07T09:04:53Z

@poppa would you do a quick PR only with this change to fix Windows CI? #15761. Better if we get that in a separate PR

patak-cat

ok, let's merge it so we can unblock CI for others

vitejs/vite#11254 vitejs/vite#15761

fix(config): Imrpoved warning root path includes bad characters

a2dcec9

Previously a warning was only issued when the root path included the `#` character, but the `?` character is equally bad.

poppa changed the title ~~fix(config): Improved warning when root path includes bad characters~~ fix(config): improved warning when root path includes bad characters Jan 31, 2024

steadily-worked mentioned this pull request Jan 31, 2024

feat: warn if ? in project root #15755

Closed

9 tasks

poppa added 2 commits January 31, 2024 13:31

chore(test): fixed typo in test description

d7907d1

chore: Conformity in test names

9bd1e3a

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.

poppa added 2 commits February 3, 2024 09:34

Merge branch 'vitejs:main' into main

15065c3

ci: trigger ci check

f0d8e57

bluwy reviewed Feb 6, 2024

View reviewed changes

Comment thread packages/vite/src/node/config.ts Outdated

Comment thread packages/vite/src/node/config.ts Outdated

Comment thread packages/vite/src/node/config.ts Outdated

Comment thread playground/config/__tests__/resolveConfig.spec.ts Outdated

bluwy added the p2-nice-to-have Not breaking anything but nice to have (priority) label Feb 6, 2024

poppa and others added 5 commits February 7, 2024 07:05

Merge branch 'vitejs:main' into main

8caaa91

test: Added expect assertions to tests

644e283

chore: Moved the "bad character check" to its own function

0c90166

chore: Updated a FIXME comment

b7653bd

chore: style code shorter

a183ac4

bluwy previously approved these changes Feb 7, 2024

View reviewed changes

patak-cat previously approved these changes Feb 7, 2024

View reviewed changes

patak-cat reviewed Feb 7, 2024

View reviewed changes

chore: improve test

80b1bf4

bluwy dismissed stale reviews from patak-cat and themself via 80b1bf4 February 7, 2024 09:20

patak-cat approved these changes Feb 7, 2024

View reviewed changes

patak-cat merged commit 1c0dc3d into vitejs:main Feb 7, 2024

sapphi-red mentioned this pull request Mar 19, 2025

feat(config): improve bad character warning #19683

Merged

sapphi-red added a commit to vitejs/vite-plugin-react that referenced this pull request Apr 1, 2025

ci: fix playwright cache

f69e06a

vitejs/vite#11254 vitejs/vite#15761

sapphi-red added a commit to vitejs/vite-plugin-react that referenced this pull request Apr 1, 2025

ci: fix playwright cache

0eb9aca

vitejs/vite#11254 vitejs/vite#15761

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(config): improved warning when root path includes bad characters#15761

fix(config): improved warning when root path includes bad characters#15761
patak-cat merged 12 commits intovitejs:mainfrom
poppa:main

poppa commented Jan 31, 2024 •

edited by bluwy

Loading

Uh oh!

bolt-new-by-stackblitz Bot commented Jan 31, 2024

Uh oh!

poppa commented Feb 1, 2024

Uh oh!

poppa commented Feb 1, 2024

Uh oh!

poppa commented Feb 1, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bluwy Feb 7, 2024

Uh oh!

patak-cat left a comment

Uh oh!

patak-cat Feb 7, 2024

Uh oh!

bluwy Feb 7, 2024

Uh oh!

patak-cat commented Feb 7, 2024

Uh oh!

patak-cat left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

poppa commented Jan 31, 2024 • edited by bluwy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context

What is the purpose of this pull request?

Before submitting the PR, please make sure you do the following

Uh oh!

bolt-new-by-stackblitz Bot commented Jan 31, 2024

Uh oh!

poppa commented Feb 1, 2024

Uh oh!

poppa commented Feb 1, 2024

Uh oh!

poppa commented Feb 1, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bluwy Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

patak-cat left a comment

Choose a reason for hiding this comment

Uh oh!

patak-cat Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

bluwy Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

patak-cat commented Feb 7, 2024

Uh oh!

patak-cat left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

poppa commented Jan 31, 2024 •

edited by bluwy

Loading