Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Error when using route with multiple parameters in single segment #547

Closed
6eDesign opened this issue Jan 16, 2019 · 7 comments
Closed

Error when using route with multiple parameters in single segment #547

6eDesign opened this issue Jan 16, 2019 · 7 comments
Labels

Comments

@6eDesign
Copy link

This issue can be reproduced by creating a route such as routes/blah.[slug].[id].html which results in an error like this one:

> Server crashed
webpack:///./__sapper__/server.js?:1
throw new Error("Module parse failed: Unexpected token (60:124)\nYou may need an appropriate loader to handle this file type.\n| \t\t\tpattern: /^\\/blah.([^\\/]+?)\\/?$/,\n| \t\t\tparts: [\n> \t\t\t\t{ name: \"blah_$slug$93_$91id\", file: \"blah.[slug].[id].html\", component: __blah_$slug$93_$91id, params: match => ({ slug].[id: d(match[1]) }) }\n| \t\t\t]\n| \t\t},");
^

Error: Module parse failed: Unexpected token (60:124)
You may need an appropriate loader to handle this file type.
|                       pattern: /^\/blah.([^\/]+?)\/?$/,
|                       parts: [
>                               { name: "blah_$slug$93_$91id", file: "blah.[slug].[id].html", component: __blah_$slug$93_$91id, params: match => ({ slug].[id: d(match[1]) }) }
|                       ]
|               },
    at eval (webpack:///./__sapper__/server.js?:1:7)
    at Object../__sapper__/server.js (C:\dev\homeadvisor\sapper-play\__sapper__\dev\server\server.js:97:1)
    at __webpack_require__ (C:\dev\homeadvisor\sapper-play\__sapper__\dev\server\server.js:21:30)
    at eval (webpack:///./src/server.js?:8:75)
    at Module../src/server.js (C:\dev\homeadvisor\sapper-play\__sapper__\dev\server\server.js:109:1)
    at __webpack_require__ (C:\dev\homeadvisor\sapper-play\__sapper__\dev\server\server.js:21:30)
    at C:\dev\homeadvisor\sapper-play\__sapper__\dev\server\server.js:85:18
    at Object.<anonymous> (C:\dev\homeadvisor\sapper-play\__sapper__\dev\server\server.js:88:10)
    at Module._compile (module.js:643:30)
    at Object.Module._extensions..js (module.js:654:10)
@Conduitry Conduitry added the bug label Jan 16, 2019
@Conduitry
Copy link
Member

I think this might be fixed with something as simple as a non-greedy wildcard here.

@Conduitry
Copy link
Member

Changing that regex to +? does seem to address this particular problem, but it breaks some existing unit tests - in particular ones checking for invalid route slug regexes. I'm not sure how to resolve this. Handling this properly (while still catching bad routes) seems to require much fancier parsing than we're currently doing. And then at that point we might be able to relax the restrictions against ()[] appearing in the slug regular expressions. I don't know. This suddenly got messy.

@chris-morgan
Copy link

A few days ago I looked into what it would take to make rest parameters work (#545), and supporting multiple parameters in a single segment will affect that patch too, since it’ll need to keep track of which group/s of the regexp needs to be split on slashes.

@Conduitry
Copy link
Member

Here's what I have currently for get_parts:

function get_parts(part: string): Part[] {
	const parts = [];
	let state = 0;
	let content = '';
	let qualifier;
	for (let i = 0; i < part.length; i++) {
		const char = part[i];
		const next = part[i + 1];
		if (state === 0) {
			if (char === '[') {
				if (content) {
					parts.push({ content, dynamic: false, qualifier: null });
				}
				state = 1;
				content = '';
			} else {
				content += char;
			}
		} else if (state === 1) {
			if (char === ']') {
				if (content) {
					parts.push({ content, dynamic: true, qualifier: null });
				}
				state = 0;
				content = '';
			} else if (char === '(') {
				state = 2;
				qualifier = '';
			} else {
				content += char;
			}
		} else if (state === 2) {
			if (char === ')' && next === ']') {
				parts.push({ content, dynamic: true, qualifier });
				state = 0;
				content = '';
				i += 1;
			} else {
				qualifier += char;
			}
		}
	}
	if (state) {
		throw new Error('this is bad');
	}
	if (content) {
		parts.push({ content, dynamic: false, qualifier: null });
	}
	return parts;
}

I don't know whether this is heading in the correct direction or not. I think it might be? There is other stuff that still needs to happen, but having this handled in a more parser-y way rather than with regexes feels saner.

@Conduitry
Copy link
Member

This has been finally fixed in 0.27.13 - thanks @JohnPeel!

@loversandcode
Copy link

loversandcode commented May 29, 2020

Does this changes somehow affect the route file names? I don't understand, cause after update from 0.27.10 to 0.27.13 some of routes in my app considered as not found routes?

@JohnPeel
Copy link
Contributor

Does this changes somehow affect the route file names? I don't understand, cause after update from 0.27.10 to 0.27.13 some of routes in my app considered as not found routes?

Can you give an example route?

The only extra condition this applies is having slugs next to each other, but that would have already been invalid before the update.

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

5 participants