docs(main): fix server.proxy doc example code#10169
docs(main): fix server.proxy doc example code#10169luckynumberke7in wants to merge 2 commits intovitejs:mainfrom luckynumberke7in:patch-1
Conversation
|
This is the working file, it had the escape character in it, but when I saved the string, it automatically disappeared (and still works like this).
This is the broken version that the docs suggest. You can see the 404 failure and that the proxy is not working. I have also set up a proxy using direct path and no |
|
Note: the working one works with and without |
|
Also, I think it's failing for the save discrepancy I mentioned above (when I saved the escape characters vanish with prettier). |
|
I should probably update the PR to be This is my first public PR, sorry if I'm all over the place! |
|
For future reference: How would I go about updating my PR? Do I have to refork and redo? or can I push another change to the current PR? |
docs/config/server-options.md
Outdated
| target: 'http://jsonplaceholder.typicode.com', | ||
| changeOrigin: true, | ||
| rewrite: (path) => path.replace(/^\/api/, '') | ||
| rewrite: (path) => path.replace('/^\/api/', '') |
There was a problem hiding this comment.
This is not correct.
path.replace() is String.prototype.replace(), which can take a regular expression as its first argument.
Your change would cause rewrite to only perform a replacement if path contained /^\/api/ literally, e.g., https://localhost:5173/^\/api/posts.
The reason your change worked in your own code is likely because it effectively prevented the rewrite from occurring, which means you don't need the rewrite in the first place. Just remove rewrite in your config.
There was a problem hiding this comment.
Ahh, that did work! Sorry, everything I found online included the rewrite, but it just was not working.
I came to the docs and came to the same conclusion since it was in options and regex.
Maybe it could be made a little more clear that proxy matches following routes (i.e. '/api/products', /api/products/1', etc.) by default using:
proxy: {
'/api': {
target: 'http://localhost:5000',
changeOrigin: true,
secure: false,
}
It was confusing for me at least.
There was a problem hiding this comment.
Yeah, I think we could have examples.
// string shorthand: http://localhost:5173/foo -> http://localhost:4567/foo
// with options: http://localhost:5173/api/bar-> http://jsonplaceholder.typicode.com/bar
There was a problem hiding this comment.
for readability, I was thinking something more along the lines of:
export default defineConfig({
server: {
proxy: {
// string shorthand, matches calls to '/foo/bar' by default
'/foo': 'http://localhost:4567',
// with options, returns http://jsonplaceholder.typicode.com/bar
'/api': {
target: 'http://jsonplaceholder.typicode.com',
changeOrigin: true,
rewrite: (path) => path.replace(/^\/api/, '')
},There was a problem hiding this comment.
the matching subsequent folders by default is what was confusing me. I thought I needed a rewrite to achieve this.
You can continue pushing changes to the PR. If the PR is not ready to be reviewed, mark it as a Draft. In my PRs, if I expect a high number of pushes (which causes a lot of notification noise for reviewers), I make my changes in a branch off my PR branch, and then merge back when ready for review. |
|
@luckynumberke7in do you want to implement #10169 (comment) in this PR? Marking this one as draft for now, once you're ready you can mark this ready for review. |
| // string shorthand, matches calls to '/foo/bar' by default | ||
| '/foo': 'http://localhost:4567', | ||
| // with options | ||
| // with options, returns http://jsonplaceholder.typicode.com/bar |
There was a problem hiding this comment.
the matching subsequent folders by default is what was confusing me. I thought I needed a rewrite to achieve this.
If this is the reason, I think it's better to update the description (line 80) because these examples are a bit ambiguous.
docs(main): fix server.proxy doc example code
wrap
path.replaceregex in a string to resemble working codeshould resolve issues like #1014, #3475, #10168
Description
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).