vikunja
/
frontend
Archived
6
39
Fork 49
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

chore: add env var to use createWebHashHistory #3313

Closed
WofWca wants to merge 1 commits from WofWca/frontend:router-hash-hostory-env into main
pull from: WofWca/frontend:router-hash-hostory-env
merge into: vikunja:main
Conversation 7 Commits 1 Files Changed 2 +6 -3
WofWca commented 2023-03-27 11:14:05 +00:00
Contributor
Copy Link

This is needed to deploy on servers like GitHub pages.

This is needed to deploy on servers like GitHub pages.
👍 1
WofWca added 1 commit 2023-03-27 11:14:07 +00:00
continuous-integration/drone/pr Build is passing Details
28b2f1aa62 chore: add env var to use `createWebHashHistory`
frederick commented 2023-03-27 11:19:32 +00:00
Member
Copy Link

Hi WofWca!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://3313-router-hash-hostory-env--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi WofWca! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://3313-router-hash-hostory-env--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
dpschen reviewed 2023-03-27 11:47:45 +00:00
src/router/index.ts
@ -1,4 +1,4 @@
import { createRouter, createWebHistory } from 'vue-router'
import { createRouter, createWebHashHistory, createWebHistory } from 'vue-router'
dpschen commented 2023-03-27 11:42:56 +00:00
Member

Do you know if vite / esbuild is intelligent enough to only bundle only one of those two history implementations imports?

Because the information should be available at build time.
If not could we check if this is possible?

Do you know if vite / esbuild is intelligent enough to only bundle only one of those two history implementations imports? Because the information should be available at build time. If not could we check if this is possible?
dpschen commented 2023-03-27 11:47:40 +00:00
Member

So what I mean is: in case esbuild isn't intelligent enough could we help it via some 'special syntax' ala:

(pseudocode)

import { createRouter } from 'vue-router'

if (import.meta.env.VITE_ROUTER_HISTORY_MODE_IS_HASH === 'true') {
  import { createWebHashHistory: createHistory } from 'vue-router'
} else {
  import { createWebHistory: createHistory } from 'vue-router'
}

and then

// [...]
history: createHistory(import.meta.env.BASE_URL),
// [...]
So what I mean is: in case esbuild isn't intelligent enough could we help it via some 'special syntax' ala: *(pseudocode)* ```ts import { createRouter } from 'vue-router' if (import.meta.env.VITE_ROUTER_HISTORY_MODE_IS_HASH === 'true') { import { createWebHashHistory: createHistory } from 'vue-router' } else { import { createWebHistory: createHistory } from 'vue-router' } ``` and then ```ts // [...] history: createHistory(import.meta.env.BASE_URL), // [...] ```
WofWca commented 2023-03-27 12:15:35 +00:00
Author
Contributor

Webpack is capable of eliminating unused imports. Not sure about others.

Webpack is capable of eliminating unused imports. Not sure about others.
src/router/index.ts
@ -82,3 +82,3 @@
const router = createRouter({
history: createWebHistory(import.meta.env.BASE_URL),
history: import.meta.env.VITE_ROUTER_HISTORY_MODE_IS_HASH === 'true'
dpschen commented 2023-03-27 11:44:06 +00:00
Member

So this will be replaced on build time but will still contain the complete import.

So this will be replaced on build time but will still contain the complete import.
dpschen requested review from dpschen 2023-03-27 11:48:00 +00:00
dpschen added the
kind/feature
 label 2023-03-27 11:48:07 +00:00
dpschen commented 2023-03-27 11:49:49 +00:00
Member
Copy Link

Can you add the new envs also to /env.d.ts?

Can you add the new envs also to `/env.d.ts`?
konrad commented 2023-03-27 11:51:23 +00:00
Owner
Copy Link

Vikunja actually used the has history mode in the beginning. We gave up using it because it would not work with query params which are used for password resets and email confirmation links.

The other problem is these emails are sent by the api without knowledge of whether the frontend uses the hash mode or the web history mode.

Since these two problems are rather fundamental, I'm afraid we can't merge this. Sorry about that.

Vikunja actually used the has history mode in the beginning. We gave up using it because it would not work with query params which are used for password resets and email confirmation links. The other problem is these emails are sent by the api without knowledge of whether the frontend uses the hash mode or the web history mode. Since these two problems are rather fundamental, I'm afraid we can't merge this. Sorry about that.
👍 1
WofWca closed this pull request 2023-03-27 12:16:11 +00:00
dpschen commented 2023-03-27 12:37:53 +00:00
Member
Copy Link

Vikunja actually used the has history mode in the beginning. We gave up using it because it would not work with query params which are used for password resets and email confirmation links.

The other problem is these emails are sent by the api without knowledge of whether the frontend uses the hash mode or the web history mode.

Since these two problems are rather fundamental, I'm afraid we can't merge this. Sorry about that.

Actually I asked myself often if it wouldn't make sense to use new routes instead for the callbacks. The problem that the API doesn't know if the frontend is using hash mode or history mode could also be easily fixed via always using the hash mode backend and in frontend something like (copied from boot.dev):

// Redirect if path begins with a hash (ignore hashes later in path)
router.beforeEach((to, from, next) => {
  // Redirect if fullPath begins with a hash (ignore hashes later in path)
  if (to.fullPath.substr(0, 2) === '/#') {
    const path = to.fullPath.substr(2);
    next(path);
    return;
  }
  next();
});
> Vikunja actually used the has history mode in the beginning. We gave up using it because it would not work with query params which are used for password resets and email confirmation links. > > The other problem is these emails are sent by the api without knowledge of whether the frontend uses the hash mode or the web history mode. > > Since these two problems are rather fundamental, I'm afraid we can't merge this. Sorry about that. Actually I asked myself often if it wouldn't make sense to use new routes instead for the callbacks. The problem that the API doesn't know if the frontend is using hash mode or history mode could also be easily fixed via always using the hash mode backend and in frontend something like (copied from [boot.dev](https://blog.boot.dev/javascript/vue-history-mode-support-legacy-hash-urls/)): ```ts // Redirect if path begins with a hash (ignore hashes later in path) router.beforeEach((to, from, next) => { // Redirect if fullPath begins with a hash (ignore hashes later in path) if (to.fullPath.substr(0, 2) === '/#') { const path = to.fullPath.substr(2); next(path); return; } next(); }); ```
👍 1
dpschen reopened this pull request 2023-03-29 20:45:17 +00:00
dpschen commented 2023-03-29 20:45:48 +00:00
Member
Copy Link

@konrad Reopening because unsure if you saw the comment :)

@konrad Reopening because unsure if you saw the comment :)
konrad commented 2023-04-01 08:34:04 +00:00
Owner
Copy Link

@dpschen You mean to add the snippet you're proposing to this PR as well?

@dpschen You mean to add the snippet you're proposing to this PR as well?
👍 1
dpschen commented 2023-04-01 09:57:20 +00:00
Member
Copy Link

@dpschen You mean to add the snippet you're proposing to this PR as well?

Yes

> @dpschen You mean to add the snippet you're proposing to this PR as well? Yes
konrad closed this pull request 2023-10-22 17:07:25 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
dpschen
Labels
Clear labels   
area/internal-code

   
changes requested

   
confirmed

   
dependencies

   
duplicate

This issue or pull request already exists

  
good first issue

   
help wanted

Need some help

  
hosting

   
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
4 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#3313
No description provided.
Delete Branch "WofWca/frontend:router-hash-hostory-env"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?