Skip to content
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: build path error on Windows #7383

Merged
merged 2 commits into from Mar 29, 2022

Conversation

j-maas
Copy link
Contributor

@j-maas j-maas commented Mar 19, 2022

Description

When the build input is specified, on Windows the build fails with an error and a weird path:

$ npm run build

> vite-project@0.0.0 build
> vite build

vite v2.8.6 building for production...
✓ 1 modules transformed.
Generated an empty chunk: "index"
rendering chunks (1)...[vite:build-html] The "fileName" or "name" properties of emitted files must be strings that are neither absolute nor relative paths, received "../../../../../C:\Users\Johannes\Code\vite-build-path-windows-reproduction\index.html".
error during build:
Error: The "fileName" or "name" properties of emitted files must be strings that are neither absolute nor relative paths, received "../../../../../C:\Users\Johannes\Code\vite-build-path-windows-reproduction\index.html".
    at error (C:\Users\Johannes\Code\vite-build-path-windows-reproduction\node_modules\rollup\dist\shared\rollup.js:198:30)
    at FileEmitter.emitFile (C:\Users\Johannes\Code\vite-build-path-windows-reproduction\node_modules\rollup\dist\shared\rollup.js:15513:24)
    at Object.generateBundle (C:\Users\Johannes\Code\vite-build-path-windows-reproduction\node_modules\vite\dist\node\chunks\dep-9c153816.js:21691:22)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)

The issue seems to be the hard-coded use of POSIX-style paths in the build script. When using the platform-appropriate path module, the issue is resolved for me.

A minimal reproduction is provided at https://github.com/j-maas/vite-build-path-windows-reproduction.

Additional context

I'm not sure, but I suspect that this might solve the similar issue from #7191. They opened #7192 to fix it, but it seems that their fix does not behave correctly, judging by the failing tests. However, if this PR is appropriate, it might be worth revisiting that issue and PR to potentially close them as fixed.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

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

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • 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.

@j-maas j-maas marked this pull request as draft March 19, 2022 19:36
@j-maas j-maas changed the title Fix build path error on Windows fix: build path error on Windows Mar 19, 2022
@j-maas
Copy link
Contributor Author

j-maas commented Mar 19, 2022

I'm not sure how to add a test for this. If you could point me to where it would be and if there is a similar test, I could have a go at adding one.

@j-maas j-maas marked this pull request as ready for review March 19, 2022 20:28
@patak-dev
Copy link
Member

We have a guide for adding new tests here https://github.com/vitejs/vite/blob/main/CONTRIBUTING.md#extending-the-test-suite. We have tests for rollup inputs here https://github.com/vitejs/vite/blob/main/packages/playground/html/vite.config.js. If your example works in plain rollup, we may want to add one path without resolving there to test this.

@j-maas
Copy link
Contributor Author

j-maas commented Mar 20, 2022

Unfortunately, I can't figure out how to make the test work. I got them running and passing, and I can even get all the tests in the html.spec.test suite to fail when in vite.config.js I rename main: resolve(__dirname, 'index.html'), to main: 'index.html'. But I don't really understand what this change is doing and I don't get useful error messages.

[...]
● unicode path › direct access

    ReferenceError: viteTestUrl is not defined

      201 |   test('direct access', async () => {
      202 |     await page.goto(
    > 203 |       viteTestUrl + '/unicode-path/中文-にほんご-한글-🌕🌖🌗/index.html'
          |       ^
      204 |     )
      205 |     expect(await page.textContent('h1')).toBe('unicode-path')
      206 |   })

      at Object.<anonymous> (packages/playground/html/__tests__/html.spec.ts:203:7)
  ● unicode path › spa fallback

    ReferenceError: viteTestUrl is not defined

      207 |
      208 |   test('spa fallback', async () => {
    > 209 |     await page.goto(viteTestUrl + '/unicode-path/中文-にほんご-한글-🌕�🌗//')
          |                     ^
      210 |     expect(await page.textContent('h1')).toBe('unicode-path')
      211 |   })
      212 | })

      at Object.<anonymous> (packages/playground/html/__tests__/html.spec.ts:209:21)


  ● Test suite failed to run

    Could not resolve entry module (index.html).

      at error (node_modules/.pnpm/rollup@2.62.0/node_modules/rollup/dist/shared/rollup.js:158:30)
      at ModuleLoader.loadEntryModule (node_modules/.pnpm/rollup@2.62.0/node_modules/rollup/dist/shared/rollup.js:22400:20)
          at async Promise.all (index 0)

Test Suites: 1 failed, 1 total  
Tests:       39 failed, 39 total
Snapshots:   0 total
Time:        6.125 s
Ran all test suites matching /[^ssr-]html/i.
 ELIFECYCLE  Command failed with exit code 1.

As I've spent a few hours on this already, I don't really want to spend much more time on this. I'd be happy to follow some specific instructions, if it doesn't take me more than 30 minutes. Otherwise, I hope it's ok that I don't add a unit test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants