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): build project path error #9793

Merged
merged 4 commits into from Aug 27, 2022
Merged

Conversation

btea
Copy link
Collaborator

@btea btea commented Aug 23, 2022

Description

fix #9771

Additional context


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.

Comment on lines 380 to 382
if (input === 'index.html') {
input = resolve(input)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't have a particular case for index.html. This will be still broken for foo.html. We need to check where input is being used which results in the paths being different on both platforms. Probably in the html plugin, when computing these paths. I think we may be missing a normalizePath. https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/html.ts#L929

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks for the explanation

@sapphi-red sapphi-red added windows only p2-edge-case Bug, but has workaround or limited in scope (priority) labels Aug 23, 2022
@patak-dev
Copy link
Member

Thanks for the fix @btea! Would you try to add a test case so we avoid regressions in the future here? I think you should be able to add index.html in the config of the assets playground to replicate the issue?

@btea
Copy link
Collaborator Author

btea commented Aug 24, 2022

@patak-dev Hello, Could you help me add the test case? I don't know how to add it. Thanks!

bluwy
bluwy previously approved these changes Aug 24, 2022
@bluwy
Copy link
Member

bluwy commented Aug 24, 2022

Whoops I didn't see the comment before approving 😅 @btea it should be setting the input like https://github.com/JiPaix/Fukayo/blob/183069a81a17b9b651261d374a253641edf59a73/packages/renderer/vite.config.js#L50 in the assets playground, which should be all that's need I think.

@btea
Copy link
Collaborator Author

btea commented Aug 24, 2022

@bluwy Hi, did I misunderstand what you meant? After I updated the code, the ci failed 😅

@patak-dev
Copy link
Member

Oh, sorry @btea, looks like index.html works if you run the repo directly, but not in our tests since root ends up being CWD. Setting the root for all tests may be a good idea moving forward, but better to work on this in another PR. Let's revert the tests change.
@JiPaix, would you confirm that this PR fixes the issue for you?

@JiPaix
Copy link

JiPaix commented Aug 24, 2022

Oh, sorry @btea, looks like index.html works if you run the repo directly, but not in our tests since root ends up being CWD. Setting the root for all tests may be a good idea moving forward, but better to work on this in another PR. Let's revert the tests change. @JiPaix, would you confirm that this PR fixes the issue for you?

@patak-dev

rollupOptions.input: 'index.html'.

  • linux: ./src/filename.ext
  • windows: ./filename.ext

same output with base: '' and base: './'.

@patak-dev
Copy link
Member

Thanks for testing @JiPaix, let see if @btea is able to reproduce it 👍🏼

@btea
Copy link
Collaborator Author

btea commented Aug 25, 2022

@patak-dev @JiPaix I tested it with wsl, the output is normal, the path does not contain src.

@patak-dev
Copy link
Member

@sapphi-red maybe you could try this one with a real Windows box there?

@sapphi-red
Copy link
Member

It's working for me.
I tested with create-vite vanilla with the following config.

export default {
  base: './',
  build: {
    rollupOptions: {
      input: 'index.html'
    }
  }
}

Build result (index.html) with both Windows and WSL:

    <script type="module" crossorigin src="./assets/index.0771aa93.js"></script>
    <link rel="stylesheet" href="./assets/index.d0964974.css">

@patak-dev
Copy link
Member

@JiPaix would you provide a minimal reproduction of your issue with this PR?

@JiPaix
Copy link

JiPaix commented Aug 27, 2022

@sapphi-red @patak-dev @btea

Nvm it works, i was doing something wrong...
sorry to waste your time 🤕

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-edge-case Bug, but has workaround or limited in scope (priority) windows only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

config base : '' produce different results depending on the platform
5 participants