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

feat(module): leverage vue-router redirect #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(module): leverage vue-router redirect #37

wants to merge 1 commit into from

Conversation

ricardogobbosouza
Copy link
Member

I put new properties in the path and redirect rules because the redirection done by the vue-router is different so the user can customize it if necessary.

For example, vue-router does not accept ^/abc in spa mode, but in universal mode this is acceptable, in spa mode /abc/(:id) this is acceptable, no longer in universal mode.

This /abc redirection per example is acceptable in both modes.

Resolve #3

@codecov
Copy link

codecov bot commented Mar 21, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@4000dc7). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##             master    #37   +/-   ##
=======================================
  Coverage          ?   100%           
=======================================
  Files             ?      2           
  Lines             ?     37           
  Branches          ?      9           
=======================================
  Hits              ?     37           
  Misses            ?      0           
  Partials          ?      0
Impacted Files Coverage Δ
lib/module.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4000dc7...f6dcff5. Read the comment docs.

@manniL
Copy link
Member

manniL commented Mar 21, 2019

Thanks so far!
I wouldn't differentiate between SPA and universal mode. Instead, I'd check whether there is a server running or not (which makes the difference of having serverMiddleware handling everything or 'falling back' to the vue-router).

@ricardogobbosouza
Copy link
Member Author

Could you give me an example of how you're thinking?

@ricardogobbosouza
Copy link
Member Author

@manniL if change

if (this.options.mode === 'spa') {

to

this.nuxt.hook('generate:before', () => {

What I tested would solve the problem well.

@manniL
Copy link
Member

manniL commented Mar 25, 2019

@ricardogobbosouza but then the extended route isn't included in the build bundle / .nuxt/router.js I think.

@ricardogobbosouza
Copy link
Member Author

@manniL It worked perfectly with the test I did local

@ricardogobbosouza
Copy link
Member Author

I will work on this change and its tests, but we will still have that divergence:

  • vue router using path-to-regexp that syntaxically/contextually different from js regex , if we don't move to that and take argument in the same form, it would be difficult to keep route working when 'proxying' to it.
    Leverage vue-router alias/redirect #3 (comment)

@manniL
Copy link
Member

manniL commented Mar 27, 2019

Hmm... right. 🤔
We should make that clear in the README I guess.

@ricardogobbosouza
Copy link
Member Author

@manniL The only way to work in both cases is to use path-to-regexp on the serverMiddleware, the same as vue-router uses and this should probably generate BREAKING CHANGE

What do you think of switching to path-to-regexp ?

@ricardogobbosouza
Copy link
Member Author

Or do we create different options for the vue-router case as I said earlier #37 (comment)

@iliyaZelenko
Copy link

What do you think of switching to path-to-regexp ?

I wrote a serverMiddleware which redirects using path-to-regexp: #77 (comment)

@fliptheweb
Copy link

Hi guys! Is it abandoned? Seems like that without this feature, the module is useless.
If only i could help to push it forward?

@@ -18,7 +18,7 @@
"access": "public"
},
"scripts": {
"dev": "nuxt test/fixture",
"dev": "nuxt test/fixture/univesal",

Choose a reason for hiding this comment

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

typo univesal >universal

@fliptheweb
Copy link

Due to lack of maintenance I forked that repo with merged client routing (thanks @ricardogobbosouza !) https://github.com/Bravado-network/nuxt-redirects

What I also added:

  • permanent: true instead of 301/302 status code (like in next.js redirects) to avoid confusion and mistakes;
  • path-to-regexp instead of 2 different scheme for client and server;
  • redirect to external urls;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Leverage vue-router alias/redirect
4 participants