fix(404): handle nested package routing#72
Conversation
* add a default +error.svelte page * refactor existing 404 logic in load to have an explicit error
| let mapping = $derived( | ||
| Object.hasOwn(all.mappings, package_name) ? all.mappings[package_name] : undefined | ||
| ); | ||
| let mapping = $derived(all.mappings[package_name]); |
There was a problem hiding this comment.
this was on purpose because package_name is user input
There was a problem hiding this comment.
I'm not sure I get your comment. It's also the URL parameter: params.package. What issue do you see and I don't @43081j ?
There was a problem hiding this comment.
I think this was for security (__proto__)?
There was a problem hiding this comment.
that this was specifically using Object.hasOwn before for security reasons.
this PR doesn't need to change it
There was a problem hiding this comment.
OK, clear! That's where my Svelte knowledge ran short. I was expecting the load function to make sure of it. Let me revert that change then. 👍
There was a problem hiding this comment.
@43081j I reverted the change. Now we can get null reference exception. What do you think?
|
i'm going to wait for @paoloricciuti to review this one. it seems the template has changed to drop the im just not sure and really Paolo needs to review this. |
There was a problem hiding this comment.
We were using the +page.ts to set prerender = false but to load the data we are using the much more ergonomic remote functions...also this logic should really be a param matcher not a load function
There was a problem hiding this comment.
I always need to refresh my knowledgeable on error pages (shame on me) but I'm pretty sure we can use a single error page for both paths by moving it in the common ancestor. Will check tomorrow
|
Hello @paoloricciuti 👋 I have no hard feeling on this PR. If you have a simpler solution, go for it. I don't want you to waste your time on mine. 😅 |
|
@jdebarochez @43081j I pushed something that should work! Let me know what you think. |
|
Had to do a couple more fix because I didn't anticipate a change in the page reflecting on the remote function. Now it should work |
|
Thanks @jdebarochez 🧡 |
4 participants
Hello 👋
In order to fix #71 , I'm attempting my first contribution:
Looking at the code, we miss a default
+error.sveltepage. In order to not duplicate the existing 404 displayed error when mapping is empty, I've decided to throw the error when data is loaded. This has the benefits to avoid conditions insrc/routes/[[scope=scope]]/[package]/+page.svelte.Let me know what should be improved, I'm not a Svelte expert. 🙏