Skip to content
This repository has been archived by the owner on Jan 17, 2021. It is now read-only.

Chainable baseURL #22

Closed
idiotWu opened this issue Feb 11, 2017 · 8 comments
Closed

Chainable baseURL #22

idiotWu opened this issue Feb 11, 2017 · 8 comments
Labels

Comments

@idiotWu
Copy link
Member

idiotWu commented Feb 11, 2017

Currently(v1.0.x), router.base() will always resolve to the origin of current page. For example:

const { router } = createServer();

router.base('/api'); // => http://localhost:3000/api

However, this will be counterintuitive when we code as:

const { router } = createServer('/api');

router.base('/v1'); // => http://localhost:3000/v1

Instead of getting a path of /api/v1, we got /v1 :(. So I'm wondering if we should resolve the given baseURL against current baseURL, just like the mount-paths in express apps.

CC @vincentbel

@idiotWu
Copy link
Member Author

idiotWu commented Feb 11, 2017

Some potential issues of chainable baseURL:

1. Syntax

Should relative paths be prefixed with '.'?

const { router } = createServer('/api');

router.base('/v1'); 
// OR
router.base('./v1');

P.S. The native URL accepts both relative and absolute paths (notice the trailing slash):

new URL('/v1', 'http://localhost:3000/api'); // => http://localhost:3000/v1
new URL('./v1', 'http://localhost:3000/api'); // => http://localhost:3000/v1
new URL('./v1', 'http://localhost:3000/api/'); // => http://localhost:3000/api/v1

2. Possible confusion

Think about the following scenario: router.base() points to another domain:

const { router } = createServer('/api'); // => http://localhost:3000/api

router.base('http://remote.com'); // => http://remote.com

@vincentbel
Copy link
Member

@idiotWu Summary of some of our discussions:

  1. Servers with different origin should use createServer, rather than router.base(because they are different servers, router.base is counter-intuitive)
// Good
const apiRouter = createServer('/api').router
const githubApiRouter = createServer('https://api.github.com').router

// Bad
const server = createServer('/api')
const apiRouter = server.router
const githubApiRouter = server.router.base('/https://api.github.com')

// Also bad
const server = createServer()
const apiRouter = server.router.base('/api')
const githubApiRouter = server.router.base('/https://api.github.com')
  1. Chainable baseURL can also be used to split routes. Example: https://gist.github.com/VincentBel/6a7d35e6a2999e2aa90d065a77831bfd

Feel free to point out faults if exists.

@idiotWu
Copy link
Member Author

idiotWu commented Feb 13, 2017

@vincentbel so the given baseURL should be regarded as a kind of "prefixes" for the path?

Ah, we have already documented it as "prefixes":

screenshot

So I think this issue is more a bug fix than a breaking feature?

@vincentbel
Copy link
Member

@idiotWu yeah, I think we don't provide router.base as a way to create a new server in purpose. So I also think it is more a bug fix.

@idiotWu
Copy link
Member Author

idiotWu commented Feb 13, 2017

@vincentbel one more thing, as we've discussed, the router.base() method is designed for "prefixes", or we can say relative paths only. So should we reject all absolute paths or just throw a warning?

router.base('http://a.com'); // die or show warning?

@vincentbel
Copy link
Member

@idiotWu I think die is better because we don't mean to support this approach.😑

@vincentbel
Copy link
Member

and warnings are just ignored by developers. 😅

@idiotWu idiotWu added bug and removed discussion labels Feb 13, 2017
@idiotWu
Copy link
Member Author

idiotWu commented Feb 15, 2017

Resolved in #23 #25

@idiotWu idiotWu closed this as completed Feb 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants