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

test: revert Prevent regressions in static file serving of files outside the root (#2358) #3015

Merged
merged 1 commit into from
Apr 16, 2021
Merged

test: revert Prevent regressions in static file serving of files outside the root (#2358) #3015

merged 1 commit into from
Apr 16, 2021

Conversation

patak-dev
Copy link
Member

Description

@ElMassimo This reverts commit ee7eb82. At least in my machine this test is failing probably because of timing issues related to when the css folder is copied to temp, so the outer asset is not there when it is trying to import it. It would also fail if the assets test is run in isolation.

Let's create a new PR later with a different strategy for testing this. I also think that this test may end up interfering with security PRs that may remove the ability to get a file out of root (except for monorepos, so that has to be taken into account).


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.

Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

okay 🤔

Do we have an issue for adding another strategy? So it will not get out of mind?

@patak-dev patak-dev merged commit e472d29 into vitejs:main Apr 16, 2021
@patak-dev
Copy link
Member Author

@Shinigami92 I am not sure if this should even be a feature in Vite after the PR to only serve files from root is merged. @ElMassimo is this something that you are using in vite-ruby? You may want to check out this PR #2850 and see if this may affect a valid use case

@ElMassimo
Copy link
Contributor

Vite Ruby relies on serving files outside of Vite's root.

As long as there is a projectRoot or workspaceRoot that can be set to the actual project root, it should not be a problem.

I don't have my computer until next week—I'll make sure to check that PR.

That said, this pull request covers a very fragile part of Vite which is related to the way paths are joined. Lack of coverage makes it likely that it will break again as it has a couple of times in the past.

@patak-dev
Copy link
Member Author

Let's look for a way to add the test back then

TobiasMelen pushed a commit to TobiasMelen/vite that referenced this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants