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

Catch all route with prefix throw Expected "0" to be defined #2505

Closed

Comments

@manniL
Copy link

manniL commented Nov 27, 2018

Version

3.0.2

Reproduction link

https://codesandbox.io/s/lykxn57kr7

Prefix routes are fine when they don't have a prefix but with a /sth/ it becomes problematic.

This causes problems especially for nuxt-i18n together with a catch-all-route (as it uses lang prefixes)

Steps to reproduce

  1. Visit the CSB/Website
  2. See warning in the console.

Info: This did not happen in 3.0.1
Related: #2496, #2503

What is expected?

No warning

What is actually happening?

Warning

@manniL
Copy link
Author

manniL commented Nov 27, 2018

Update: @clarkdo fixed that in Nuxt via https://github.com/nuxt/nuxt.js/pull/4394/files#diff-90f8231837497e514ba4e8e477131204R437

Maybe we could apply the same fix (as the original part is still in the code as is https://github.com/nuxt/nuxt.js/pull/4394/files) or revert the change? ☺️

@posva
Copy link
Member

posva commented Nov 27, 2018

I think it's better to modify the check done to display the warning 😆

@manniL
Copy link
Author

manniL commented Nov 27, 2018

@posva Well,

throw new TypeError('Expected "' + token.name + '" to be defined')
is part of the pathToRegexp package.

Related src file:

/* @flow */
import { warn } from './warn'
import Regexp from 'path-to-regexp'
// $flow-disable-line
const regexpCompileCache: {
[key: string]: Function
} = Object.create(null)
export function fillParams (
path: string,
params: ?Object,
routeMsg: string
): string {
try {
const filler =
regexpCompileCache[path] ||
(regexpCompileCache[path] = Regexp.compile(path))
return filler(params || {}, { pretty: true })
} catch (e) {
if (process.env.NODE_ENV !== 'production') {
warn(false, `missing param for ${routeMsg}: ${e.message}`)
}
return ''
}
}

@posva
Copy link
Member

posva commented Nov 27, 2018

I mean we should be able to do it in the warning check. I haven't checked it at runtime, but I'm talking about util/params.js

@rchl
Copy link
Contributor

rchl commented Nov 28, 2018

This bug triggers when calling router.resolve() with params: {pathMatch: 'foo'} when matching against existing routes that have * wildcard. That makes me wonder what will happen if we are trying to resolve with params: {pathMatch: 'foo'} and there is actual route that defines param called pathMatch. It would then match, which would likely be incorrect. It might be a bit of a stretch but...

I wonder if the special case of 'unnamed' param might need a more tailored approach here.

@rchl
Copy link
Contributor

rchl commented Nov 29, 2018

(BTW. This bug is a recent regression related to pathMatch stuff so I would consider it more than just an 'improvement'. It means that with perfectly valid setup, one can now get warnings when matching routes.)

posva pushed a commit that referenced this issue Dec 22, 2018
posva added a commit that referenced this issue Dec 22, 2018
@posva posva reopened this Dec 22, 2018
@posva posva closed this as completed in e224637 Dec 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment