Fix Redirect issue when using baseURL #1584

sidharthv96 · 2021-05-29T12:54:25Z

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

It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
This message body should clearly illustrate what problems it solves.
Ideally, include a test that fails without this PR but passes with it.

Tests

Run the tests with pnpm test and lint the project with pnpm lint

Changesets

If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts

* 'master' of https://github.com/sveltejs/kit: Version Packages (next) (sveltejs#1543) type fixes for adapter-node and adapter-static (sveltejs#1578) Upgrade to Vite 2.3.3 (sveltejs#1580) fix: improve getRawBody parsing & handle error(s) (sveltejs#1528) create-svelte: add svelte-check for TS (sveltejs#1556) pass validated svelte config to adapters (sveltejs#1559) types: group related and reduce potential inconsistencies (sveltejs#1539) Use sveltekit tag on StackOverflow (sveltejs#1558) Fix create-svelte build-template script (sveltejs#1555) Remove err param from Polka .listen() callback (sveltejs#1550) bump: polka and sirv versions (sveltejs#1548) svelte-kit package (sveltejs#1499)

* 'master' of github.com:sidharthv96/kit: chore: Add changeset for favicon Add favicon to skeleton template

This reverts commit 9dabbe0.

sidharthv96 · 2021-05-29T13:36:20Z

Should I be adding tests to packages/kit/test/apps/basics/src/routes/routing/_tests.js ?

sidharthv96 · 2021-05-29T13:56:38Z

Seems like all the tests are running with a config with empty paths.base.

Got this when I grepped the output after logging the config being passed to the tests.

"base": "",
"base": "",
"base": "",
"base": "",

Is there any tests which utilise 2 configs which I can refer to ?

dummdidumm · 2021-05-29T17:15:55Z

I think you need to create a new top-level-app so to speak (below tests) where you can create a svelte.config with a baseUrl

* upstream/master: Add favicon to skeleton template (sveltejs#1514)

sidharthv96 · 2021-05-29T19:17:31Z

@dummdidumm Tried creating a new app, but it's failing when I try to load /basePath/valid.

I'm adding that folder into this PR. I've named it aa-routing just so that it's run first and I can see failures fast.

sidharthv96 · 2021-05-29T20:19:23Z

packages/kit/src/runtime/client/router.js

@@ -242,7 +242,7 @@ export class Router {

 			if (incorrect) {
 				info.path = has_trailing_slash ? info.path.slice(0, -1) : info.path + '/';
-				history.replaceState({}, '', `${info.path}${location.search}`);
+				history.replaceState({}, '', `${this.base}${info.path}${location.search}`);


This is the main fix in the PR.
Rest are UX improvements for CLI/Config validations.

Rich-Harris

thank you — just left a few comments

packages/kit/src/cli.js

Rich-Harris · 2021-05-29T21:34:13Z

packages/kit/test/apps/aa-routing/src/app.html

@@ -0,0 +1,12 @@
+<!DOCTYPE html>


rather than creating a new top-level app, could we move these into test/apps/options? it will mean updating the paths in the existing tests in that app, but i think that's preferable to creating a new app, since that will have a noticeable impact on how long tests take to run overall

packages/kit/test/apps/aa-routing/src/routes/valid/_tests.js

.changeset/soft-bikes-bake.md

sidharthv96 · 2021-05-30T03:06:18Z

@Rich-Harris the Options tests have been modified. But they still won't pass as the base path is not properly used in the dev and build servers.
I tried adding the base config in vite server, that did fix some tests, but broke others.
When I tested this with my app, index page would not load and threw The server is configured with a public base URL of /mermaid-live-editor/ - did you mean to visit /mermaid-live-editor/mermaid-live-editor instead? which came from a vite middleware.
https://github.com/sveltejs/kit/pull/1584/files#diff-ed48923f0655f935bee505cdcbf40212129ff49a89c2c9d752bd63aa1e6282c4

Rich-Harris · 2021-05-30T19:51:24Z

Ah, of course — the base path isn't reflected locally during dev (or ideally during preview — #1154 / #1155), and as such the tests shouldn't account for it.

We could change that behaviour so that the base path is reflected, I haven't spent much time thinking about which is preferable. Either way, in the meantime we should a) remove the tests from this PR, and b) remove all the base occurrences in cli.js, leaving just the changes in load_config and client/router.js.

sidharthv96 · 2021-05-31T06:18:56Z

Raised #1608 with just the changes mentioned.

benmccann · 2021-06-11T02:47:18Z

@sidharthv96 thanks for you contribution. I merged your other PR together with a related one. If you'd like to continue with this one it would need to be rebased now

benmccann · 2021-07-11T01:47:07Z

@sidharthv96 did you want to rebase and continue with this PR or close it?

* 'master' of github.com:sidharthv96/kit: (1114 commits) Version Packages (next) (sveltejs#1858) Bump vite-plugin-svelte to 1.0.0-next.12 (sveltejs#1869) [fix] preserve user defined config and files on `svelte-kit package` (sveltejs#1735) [fix] handle undefined body on endpoint output (sveltejs#1808) [fix] copy essentials files from root on packaging (sveltejs#1747) [docs] sort config alphabetically (sveltejs#1867) add config.kit.package.emitTypes option (sveltejs#1852) [fix] add $lib alias to js/tsconfig (sveltejs#1860) Pass along custom properties added to Error (sveltejs#1821) Version Packages (next) (sveltejs#1840) Improve grammar in packages FAQ Docs for writing an adapter (sveltejs#1846) Additional documentation around pnpx changeset usage [feat] expose Vite.js `mode` from `$app/env` (sveltejs#1789) Service worker files exclusion support (sveltejs#1645) chore: Enable `vite.server.fs.strict` internally by default (sveltejs#1842) Test with the latest version of Svelte (sveltejs#1848) [docs] don't need to run pnpm install twice Improve HN example docs [fix] correct `ReadOnlyFormData` generator implementation (sveltejs#1837) ...

changeset-bot · 2021-07-11T07:30:11Z

⚠️ No Changeset found

Latest commit: d1179e2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

benmccann · 2021-07-30T05:21:45Z

@sidharthv96 this PR would still need to be rebased. I may close this PR as stale if I don't hear from you. In that case we can always reopen later

@ts-expect-error

…ePath * 'master' of https://github.com/sveltejs/kit: (85 commits) [chore] minor simplification of `parse_body` (sveltejs#2043) Version Packages (next) (sveltejs#2026) [chore] deduplicate config types (sveltejs#2030) [chore] remove invalid css declaration (sveltejs#2038) [fix] correctly pass Vite options in preview mode (sveltejs#2036) [feat] Ignore adapter build files (sveltejs#1924) [chore] Upgrade Rollup Don't check external links on prerender (sveltejs#1679) Version Packages (next) (sveltejs#2017) [chore] convert remaining type aliases (sveltejs#2018) [feat] explicitly set compilerOptions.hydratable to config.kit.hydrate (sveltejs#2024) [feat] More powerful and configurable rendering options (sveltejs#2008) [chore] typecheck example (sveltejs#2019) Change config `prerender.force` to `prerender.onError` (sveltejs#2007) [chore] prefer interfaces to types (sveltejs#2010) [docs] minor wording improvement in migration guide (sveltejs#2006) [chore] test debugging improvements [docs] fix typo (sveltejs#2003) [chore] use @ts-expect-error instead of @ts-ignore (sveltejs#1999) Version Packages (next) (sveltejs#1996) ...

sidharthv96 · 2021-07-31T09:08:56Z

Ah, of course — the base path isn't reflected locally during dev (or ideally during preview — #1154 / #1155), and as such the tests shouldn't account for it.

We could change that behaviour so that the base path is reflected, I haven't spent much time thinking about which is preferable. Either way, in the meantime we should a) remove the tests from this PR, and b) remove all the base occurrences in cli.js, leaving just the changes in load_config and client/router.js.

@Rich-Harris should the behaviour be changed to reflect base path ?

Rich-Harris · 2021-07-31T23:13:35Z

Now that I take another look at this I'm actually unclear what problem it solves. #1583 appears to be a duplicate of #1476, which was fixed by #1666. Am I missing something, or can this be closed? Thanks

benmccann · 2021-08-09T22:01:57Z

I'm going to go ahead and close this since there hasn't been any response. We can reopen if necessary

sidharthv96 added 7 commits May 21, 2021 10:52

Add favicon to skeleton template

d3e2abd

chore: Add changeset for favicon

7d2f668

Fix redirection issue sveltejs#1583

9dabbe0

Merge branch 'master' of github.com:sidharthv96/kit into fix/baseURL

61255b5

* 'master' of github.com:sidharthv96/kit: chore: Add changeset for favicon Add favicon to skeleton template

Fix Redirect issue when using baseURL sveltejs#1583

d02c285

Add changeset

2b1b978

sidharthv96 changed the title ~~Fix Redirect issue when using baseURL #1583~~ Fix Redirect issue when using baseURL May 29, 2021

This comment has been minimized.

Sign in to view

Revert "Fix redirection issue sveltejs#1583"

aa3cfbd

This reverts commit 9dabbe0.

This comment has been minimized.

Sign in to view

Merge remote-tracking branch 'upstream/master' into fix/basePath

d022f54

* upstream/master: Add favicon to skeleton template (sveltejs#1514)

sidharthv96 added 3 commits May 30, 2021 00:48

Try adding test

cde1c51

add Validation for paths.base

75fdde7

Add basePath support to CLI

4632240

sidharthv96 commented May 29, 2021

View reviewed changes

Rich-Harris requested changes May 29, 2021

View reviewed changes

sidharthv96 added 2 commits May 30, 2021 07:42

Refactor for matching code style.

31e21f7

Modify options tests to contain base-path

4dec03a

benmccann mentioned this pull request Jun 4, 2021

Fix redirect URLs when removing/adding trailing slash with paths.base set #1608

Closed

5 tasks

benmccann added the awaiting submitter label Jul 11, 2021

sidharthv96 added 2 commits July 11, 2021 12:55

Merge branch 'sveltejs:master' into master

dc9e037

benmccann force-pushed the master branch from 3e3e6ec to d0f2afd Compare July 25, 2021 14:15

benmccann force-pushed the master branch from 06d2da2 to e8b391b Compare August 2, 2021 19:20

benmccann closed this Aug 9, 2021

benmccann mentioned this pull request Dec 3, 2021

Redirect issue when using baseURL #1583

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Redirect issue when using baseURL #1584

Fix Redirect issue when using baseURL #1584

sidharthv96 commented May 29, 2021 •

edited by dummdidumm

Loading

This comment has been minimized.

This comment has been minimized.

sidharthv96 commented May 29, 2021

sidharthv96 commented May 29, 2021

dummdidumm commented May 29, 2021

sidharthv96 commented May 29, 2021

sidharthv96 May 29, 2021

Rich-Harris left a comment

Rich-Harris May 29, 2021

sidharthv96 commented May 30, 2021

Rich-Harris commented May 30, 2021

sidharthv96 commented May 31, 2021

benmccann commented Jun 11, 2021

benmccann commented Jul 11, 2021

changeset-bot bot commented Jul 11, 2021 •

edited

Loading

benmccann commented Jul 30, 2021

sidharthv96 commented Jul 31, 2021

Rich-Harris commented Jul 31, 2021

benmccann commented Aug 9, 2021

Fix Redirect issue when using baseURL #1584

Fix Redirect issue when using baseURL #1584

Conversation

sidharthv96 commented May 29, 2021 • edited by dummdidumm Loading

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

Tests

Changesets

This comment has been minimized.

This comment has been minimized.

sidharthv96 commented May 29, 2021

sidharthv96 commented May 29, 2021

dummdidumm commented May 29, 2021

sidharthv96 commented May 29, 2021

sidharthv96 May 29, 2021

Choose a reason for hiding this comment

Rich-Harris left a comment

Choose a reason for hiding this comment

Rich-Harris May 29, 2021

Choose a reason for hiding this comment

sidharthv96 commented May 30, 2021

Rich-Harris commented May 30, 2021

sidharthv96 commented May 31, 2021

benmccann commented Jun 11, 2021

benmccann commented Jul 11, 2021

changeset-bot bot commented Jul 11, 2021 • edited Loading

⚠️ No Changeset found

benmccann commented Jul 30, 2021

sidharthv96 commented Jul 31, 2021

Rich-Harris commented Jul 31, 2021

benmccann commented Aug 9, 2021

sidharthv96 commented May 29, 2021 •

edited by dummdidumm

Loading

changeset-bot bot commented Jul 11, 2021 •

edited

Loading