TanStack•10mo ago

harsh-harlequin

Are links supported in createRoute(), and not just createRootRoute()?

We seem to not to be able to make these work on normal routes, and only have them work on the root route.

25 Replies

stormy-gold•10mo ago

can you please provide a minimal complete example?

harsh-harlequinOP•10mo ago

I can try, but this is being a pain to create on stackblitz

harsh-harlequinOP•10mo ago

I believe this is an example https://stackblitz.com/edit/tanstack-router-bwgczo?file=app%2Froutes%2Fposts.index.tsx

StackBlitz

Router Start Basic Example (forked) - StackBlitz

Run official live example code for Router Start Basic, created by Tanstack on StackBlitz

harsh-harlequinOP•10mo ago

interestingly links work in createFileRoute() and createRootRoute() but doesnt appear to pass through in createRoute() however meta does

stormy-gold•10mo ago

really? why? I found stackblitz to be quite well working . so you are using start? not router?

harsh-harlequinOP•10mo ago

its mainly my lack of practice with it. I lost the work 3 times 🙂 this is in router I believe we are not using start

stormy-gold•10mo ago

then this is not used at all in router

harsh-harlequinOP•10mo ago

head is exposed on create route and is accessible through router match, and the meta are added to it, however links are available on match when using rootroute, but not when using others I don't believe this is true, you can see the head() in router where start extracts it from

harsh-harlequinOP•10mo ago

I think its this line https://github.com/TanStack/router/pull/2571/files#diff-2ac234f610fa0e0c2146252c10a5266af9c5b6d0b63659a0fa4c9f2c4472596aR2530 I would have expected this to add links and scripts as well, but it only copys meta over @Sean Cassiere any chance you could take a look as I think this was your PR a few days ago.

GitHub

feat(start): move scripts,links, and meta to the head by Se...

Closes #2426 As discussed in #2426, this change adds a new method named head onto Route. This function takes in the same parameters as meta and returns the values used for scripts, links, and meta....

harsh-harlequinOP•10mo ago

We've just locally done the patch and it seems to fix up the issue.

diff --git a/src/router.ts b/src/router.ts
index 20c33ec42a5002c23844cca05682cd08e36163ad..fc9c8d819998458c285a72119f20781837cea93d 100644
--- a/src/router.ts
+++ b/src/router.ts
@@ -2518,7 +2518,7 @@ export class Router<
                           // so we need to wait for it to resolve before
                           // we can use the options
                           await route._lazyPromise
-
+                         
                           await potentialPendingMinPromise()
 
                           const headFnContent = route.options.head?.({
@@ -2527,7 +2527,10 @@ export class Router<
                             params: this.getMatch(matchId)!.params,
                             loaderData,
                           })
+                          
                           const meta = headFnContent?.meta
+                          const links = headFnContent?.links
+                          const scripts = headFnContent?.scripts
 
                           const headers = route.options.headers?.({
                             loaderData,
@@ -2541,6 +2544,8 @@ export class Router<
                             updatedAt: Date.now(),
                             loaderData,
                             meta,
+                            links,
+                            scripts,
                             headers,
                           }))
                         } catch (e) {

diff --git a/src/router.ts b/src/router.ts
index 20c33ec42a5002c23844cca05682cd08e36163ad..fc9c8d819998458c285a72119f20781837cea93d 100644
--- a/src/router.ts
+++ b/src/router.ts
@@ -2518,7 +2518,7 @@ export class Router<
                           // so we need to wait for it to resolve before
                           // we can use the options
                           await route._lazyPromise
-
+                         
                           await potentialPendingMinPromise()
 
                           const headFnContent = route.options.head?.({
@@ -2527,7 +2527,10 @@ export class Router<
                             params: this.getMatch(matchId)!.params,
                             loaderData,
                           })
+                          
                           const meta = headFnContent?.meta
+                          const links = headFnContent?.links
+                          const scripts = headFnContent?.scripts
 
                           const headers = route.options.headers?.({
                             loaderData,
@@ -2541,6 +2544,8 @@ export class Router<
                             updatedAt: Date.now(),
                             loaderData,
                             meta,
+                            links,
+                            scripts,
                             headers,
                           }))
                         } catch (e) {

wise-white•10mo ago

If you've got a fix for your issue, please open a PR. I'll review and merge. As far as I know, this change only really changed the API interface. Didn't make any intentional functionality changes here though.

stormy-gold•10mo ago

@Mat Clayton - Mixcloud what I meant by "this is not used by router" is that router might invoke those functions, but router does not apply DOM modifications based on it

harsh-harlequinOP•10mo ago

@Manuel Schiller I’m aware, we have our own ssr framework, similar to start which extracts these and uses them.

stormy-gold•10mo ago

ah ok

harsh-harlequinOP•10mo ago

@Sean Cassiere we can try and do it next week, we’ve applied that patch to our local build and it fixes it for us. But it’ll take time to pull a PR together Not sure if it’s a change of functionality. We just noted it wasn’t there, might have been previously, but I couldn’t say

stormy-gold•10mo ago

are you considering to switch to start in the future?

harsh-harlequinOP•10mo ago

Not sure, there a bit of complexity to it. Like we have 4-5 ports/sites running in parallel. And we don’t need all of start. It’s a consideration but the team is advising no right now.

stormy-gold•10mo ago

I am just mentioning this because the feature you use here is tailored for start. so it might change again when starts impl changes

manual-pink•10mo ago

Hi all, here is a pr. I have not managed to run tests and its not entirely clear what should be run. https://github.com/TanStack/router/pull/2854 @Manuel Schiller I must admit, that hearing that properties exposed on router are only for direct application via "start" is pretty scary. We are not currently in a position where we can migrate to "start" although in the long term if would be good to be in alignment. Is there a further discussion here to be had around framework and library, I should be able to use the library functionality without having to prescribe myself to follow the rules of the start framework. @Tanner Linsley I would be keen to know your thoughts on the above. p.s. Happy to progress this PR and fix up any potential test failures BUT again, its not entirely clear how to run tests - any guidance would be appreciated - theres a bunch of test run methods across the various package.json files and a pointer to the correct set to run would be appreciated. Cheers!

GitHub

Ensure links and scripts are available on routes created with `crea...

We found that creating a route with createRoute would not expose links and scripts where as when creating a route with createFileRoute the link and script properties would exist as expected. We exp...

manual-pink•10mo ago

Thank you for following up on this @Sean Cassiere @Manuel Schiller . I can confirm using the build generated by the pr above works as expected. Thank you again to @Sean Cassiere for getting test cases in place for this. Do we think we can get this into the next release?

wise-white•10mo ago

I think there's just a pending question from @Manuel Schiller. On my side its looking good.

manual-pink•10mo ago

So I took another look in to this and I could repro this issue across all routes in one way or another. I still haven't managed to dissect it fully but I would be interested to know if we can programatically determine if this is a true fix. e.g. removing the code and running test cases. Would be keen to get this through as we are currently sitting on the "patched" version based on this pr.

stormy-gold•10mo ago

if we fully understand this, then sure we'll merge this ASAP

conscious-sapphire•10mo ago

Can somebody catch me up here? Anything I need to do or clear up?

stormy-gold•10mo ago

nothing to do for you here we have a fix for a bug which somehow only occurs in createRoute and not in createFileRoute (or at least they behave differently) I want to understand WHY those route definitions behave differently as they should not (at least for this particular feature)

Gaming

Programming

Are links supported in createRoute(), and not just createRootRoute()?

Did you find this page helpful?