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

Remix sitemap example does not work with Remix+Vite #7

Open
684efs3 opened this issue Dec 6, 2023 · 36 comments
Open

Remix sitemap example does not work with Remix+Vite #7

684efs3 opened this issue Dec 6, 2023 · 36 comments

Comments

@684efs3
Copy link

684efs3 commented Dec 6, 2023

I get the error:

@remix-run/dev/server-build is not meant to be used directly from node_modules. It exists to provide type definitions for a virtual module provided by the Remix compiler at build time.
@lpsinger
Copy link
Member

lpsinger commented Dec 6, 2023

Did you intend to open this issue in https://github.com/nasa-gcn/remix-seo?

@684efs3
Copy link
Author

684efs3 commented Dec 6, 2023

Yes, but it looks like there's no way to open an issue there...?

@684efs3
Copy link
Author

684efs3 commented Dec 6, 2023

remix-run/remix#8126

From the Remix team: "Added a remix-run/remix#8122 (comment) stating that we won't be supporting @remix-run/dev/server-build virtual module for Vite"

remix-run/remix#8122 (comment)

We don't have plans to support @remix-run/dev/server-build virtual module with Vite. Instead, Remix will exposes the server build module ID directly (#8120).

But if you wanted to get a programmatic reference to the build (not the build module ID), you can dynamically import it:

let build = await import("./build/server/index.js")

We should also emit ./build/server/index.d.ts alongside the server build so that you get types out-of-the-box when importing the server build.

@lpsinger lpsinger transferred this issue from nasa-gcn/gcn.nasa.gov Dec 6, 2023
@lpsinger
Copy link
Member

lpsinger commented Dec 6, 2023

I might need some help with this one in the form of a PR, as I am not yet familiar with Vite.

@toddheslin
Copy link

toddheslin commented Jan 22, 2024

For anyone wondering, the simple fix for this is this for your sitemap[.]xml.ts file:

import { generateSitemap } from '@nasa-gcn/remix-seo'
import type { LoaderFunctionArgs } from '@remix-run/node'

export async function loader({ request }: LoaderFunctionArgs) {
  //@ts-ignore - This may fail during docker build typecheck if the app isn't build yet
  let build = await (
    import.meta.env.DEV
    ? import("../../build/server/index.js")
    : import(
      /* @vite-ignore */
      import.meta.resolve("../../build/server/index.js"
    )))

  //@ts-ignore - No defs for this
  return generateSitemap(request, build.routes, {
    siteUrl: 'https://buy.loungepair.com',
  })
}

Edit: after writing and submitting this comment I found it worked differently in production. Updated my example here if anyone saw it before the update.

@toddheslin
Copy link

Just another note about when using vite. You'll probably end up with an error like:

[commonjs--resolver] Server-only module referenced by client
...
  Remix automatically removes server-code from these exports:
    `loader`, `action`, `headers`
...
See https://remix.run/docs/en/main/future/vite#splitting-up-client-and-server-code

I fixed this using the https://www.npmjs.com/package/vite-env-only package as suggested in the remix docs.
Make sure you set it up in your vite.config.ts file, and then your handle will look like this:

export const handle = {
  getSitemapEntries: serverOnly$(async () => {
    const { airports } = await getLoungesDirectory()
    return airports.map((airport) => {
      return { route: `/at/${airport.iata}/`, priority: 0.8 }
    })
  }),
}

@zifahm
Copy link

zifahm commented Jan 22, 2024

@toddheslin I tried following what you recommended

I keep getting the same erro while doing the import as well

import { type LoaderFunctionArgs } from "@remix-run/node";

export async function loader({ request }: LoaderFunctionArgs) {
  let build = await import("../../build/server/index.js");
  console.log({ build });
  return null;
  // return generateSitemap(request, routes, {
  //   siteUrl: getDomainUrl(request),
  //   headers: {
  //     "Cache-Control": `public, max-age=${60 * 5}`,
  //   },
  // });
}

Error

@remix-run/dev/server-build is not meant to be used directly from node_modules.

This is really werid I thought importing the build would solve the issue!

@zifahm
Copy link

zifahm commented Jan 22, 2024

I solved this problem

I had build the version where things was already in error where I imported @remix-run/dev

creating a new buid without importing @remix-run/dev doesnt produce any error anymore

@684efs3
Copy link
Author

684efs3 commented Jan 23, 2024

This works, however, I am using Remix flat routes, and the generated urls in the sitemap will have extra slashes, e.g. https://abc.com///xyz.

I believe this library is assuming that folders like _test+ is a path, when it is not.

Is there a way to remove these? Maybe use a regex to condense any repeated slashes (/) into a single slash?

@toddheslin
Copy link

@684efs3 was this working before upgrading to vite? Feels like it is specifically an issue when using this package with remix-flat-routes. But if it's specifically vite, this line of my solution:

  return generateSitemap(request, build.routes, { ...}

shows that you are passing in build.routes. So I'd just map over the routes and change object before it's passed into generateSitemap

@lpsinger
Copy link
Member

lpsinger commented Jan 23, 2024

@toddheslin, would you please share a working sample project with vite and remix-seo? I tried to create one from the remix templates earlier today but didn't get very far.

@684efs3
Copy link
Author

684efs3 commented Jan 23, 2024

@toddheslin This is my first time using this package, and it is with Remix+Vite and remix-flat-routes. I did not use this package in the past.

@lpsinger
Copy link
Member

@684efs3 was this working before upgrading to vite? Feels like it is specifically an issue when using this package with remix-flat-routes.

Yes, it is. Here's a working example with remix-seo and remix-flat-routes, but without vite. https://github.com/lpsinger/remix-flat-routes-seo

@zifahm
Copy link

zifahm commented Jan 24, 2024

@684efs3 was this working before upgrading to vite? Feels like it is specifically an issue when using this package with remix-flat-routes.

Yes, it is. Here's a working example with remix-seo and remix-flat-routes, but without vite. https://github.com/lpsinger/remix-flat-routes-seo

I think what he is trying to say is when he tries to colocate routes with a folder like _text+, there seems to be extra slashes introduced.

In your wroking example, there isnt a folder path created for your routes eg: here _auth+
https://github.com/epicweb-dev/epic-stack/tree/main/app/routes/_auth%2B

@684efs3 This error needs to be posted in the remix-flat-routes isssue tab, not here i suppose

@lpsinger
Copy link
Member

@zifahm, @684efs3, I am still struggling to reproduce this. I am trying to figure out if this is unique to vite or not. Can you make a PR for https://github.com/lpsinger/remix-flat-routes-seo that reproduces the extra slashes?

@zifahm
Copy link

zifahm commented Jan 25, 2024

@lpsinger I just did a PR with vite config, I couldn'nt find any extra slashes!

@zifahm
Copy link

zifahm commented Jan 27, 2024

@toddheslin im having issues with this in production

import(
      /* @vite-ignore */
      import.meta.resolve("../../build/server/index.js"
    ))

I get this error in sentry
(intermediate value).resolve is not a function

not sure what to do here! I thought the file path must have been the problem, tried a lot of variations, nothting works.

@zifahm
Copy link

zifahm commented Jan 27, 2024

@toddheslin im having issues with this in production

import(
      /* @vite-ignore */
      import.meta.resolve("../../build/server/index.js"
    ))

I get this error in sentry (intermediate value).resolve is not a function

not sure what to do here! I thought the file path must have been the problem, tried a lot of variations, nothting works.

Fixed the problem!

this was enough for prod too

  const build = (await import(
    "../../build/server/index.js"
  )) as unknown as ServerBuild;

@pchmn
Copy link

pchmn commented Feb 5, 2024

@lpsinger I created a PR to reproduces the bug with remix-flat-routes and extra slashes. The problem probably comes from remix-flat-routes when using a folder like _test+ and a layout (_test+/_layout.tsx). The generated build.routes will have _test+/_layout.tsx (instead of root) as parentId for all routes under _test+. And it's not vite related either.

To fix this I just added this to my sitemap route :

// sitemap[.]xml.ts
import { generateSitemap } from "@nasa-gcn/remix-seo";
import { routes } from "@remix-run/dev/server-build";
import type { LoaderFunctionArgs } from "@remix-run/node";

export function loader({ request }: LoaderFunctionArgs) {
  // Remove the _layout parent id from the routes
  for (const key in routes) {
    const route = routes[key];
    if (route.parentId?.endsWith('_layout')) {
      route.parentId = routes[route.parentId].parentId;
    }
  }
  return generateSitemap(request, routes, {
    siteUrl: "https://balavishnuvj.com",
  });
}

@pchmn
Copy link

pchmn commented Feb 5, 2024

After all, I don't know if it happens only with _layout.tsx files, so I made an utility function to retrieve "real" parent id:

// sitemap[.]xml.ts
import { generateSitemap } from "@nasa-gcn/remix-seo";
import { routes } from "@remix-run/dev/server-build";
import type { LoaderFunctionArgs } from "@remix-run/node";
import type { ServerBuild } from "@remix-run/server-runtime";

type Routes = ServerBuild['routes'];
type Route = Routes[keyof Routes];

export function loader({ request }: LoaderFunctionArgs) {
  for (const key in routes) {
    const route = routes[key];
    route.parentId = findRealParentId(route, routes);
  }
  return generateSitemap(request, routes, {
    siteUrl: "https://balavishnuvj.com",
  });
}

function findRealParentId(route: Route, routes: Routes) {
  if (route.parentId) {
    const parentRoute = routes[route.parentId];
    if (parentRoute.path !== undefined) {
      return parentRoute.id;
    }
    return findRealParentId(parentRoute, routes);
  }
  return route.id === 'root' ? undefined : 'root';
}

@684efs3
Copy link
Author

684efs3 commented Feb 14, 2024

Thanks a lot @pchmn! Here's the slightly modified code for Remix+Vite.

import { generateSitemap } from '@nasa-gcn/remix-seo'

function findRealParentId (route, routes) {
  if (route.parentId) {
    const parentRoute = routes[route.parentId]
    if (parentRoute.path !== undefined) {
      return parentRoute.id
    }
    return findRealParentId(parentRoute, routes)
  }
  return route.id === 'root' ? undefined : 'root'
}

export async function loader ({ request }) {
  //@ts-ignore - This may fail during docker build typecheck if the app isn't build yet
  let build = await (import.meta.env.DEV
    ? import('../../build/server/index.js')
    : import(
        /* @vite-ignore */
        import.meta.resolve('../../build/server/index.js')
      ))

  for (const [key, value] of Object.entries(build.routes)) {
    try {
      value.parentId = findRealParentId(value, build.routes)
    } catch (error) {
      console.log(error)
    }
  }

  return generateSitemap(request, build.routes, {
    siteUrl: "https://balavishnuvj.com",
  })
}

@toddheslin
Copy link

Thanks @684efs3
For those using typescript, I've added some types here and also made a shallow copy of the build.routes so we aren't mutating the underlying value...although I doubt this is actually the same memory reference as what is used elsewhere in the program due to the import syntax. But still, best to make a copy:

import { generateSitemap } from '@nasa-gcn/remix-seo'
import type { LoaderFunctionArgs } from '@remix-run/node'

function findRealParentId(
  route: Record<string, unknown>,
  routes: Record<string, Record<string, unknown>>,
) {
  if (route.parentId && typeof route.parentId === 'string') {
    const parentRoute = routes[route.parentId]
    if (parentRoute.path !== undefined) {
      return parentRoute.id
    }
    return findRealParentId(parentRoute, routes)
  }
  return route.id === 'root' ? undefined : 'root'
}

export async function loader({ request }: LoaderFunctionArgs) {
  const build = (await (import.meta.env.DEV
    ? //@ts-ignore - This may fail during docker build typecheck if the app isn't build yet
      import('../../build/server/index.js')
    : import(
        /* @vite-ignore */
        import.meta.resolve('../../build/server/index.js')
      ))) as { routes: Record<string, Record<string, unknown>> }

  const routesCopy = { ...build.routes }
  for (const [_, value] of Object.entries(routesCopy)) {
    try {
      value.parentId = findRealParentId(value, routesCopy)
    } catch (error) {
      console.log(error)
    }
  }

  //@ts-ignore - No defs for this
  return generateSitemap(request, routesCopy, {
    siteUrl: 'https://buy.loungepair.com',
  })
}

@lpsinger
Copy link
Member

By the way, I've opened #8 to track the issue with multiple slashes, which seems to be separate from the issue of proper integration with Vite.

@pchmn
Copy link

pchmn commented Feb 15, 2024

@toddheslin for typings I personaly use directly ServerBuild from @remix-run/node:

import { generateSitemap } from '@nasa-gcn/remix-seo';
import type { LoaderFunctionArgs, ServerBuild } from '@remix-run/node';

type Routes = ServerBuild['routes'];
type Route = Routes[keyof Routes];

function findRealParentId(route: Route, routes: Routes) {
  if (route.parentId) {
    const parentRoute = routes[route.parentId];
    if (parentRoute.path !== undefined) {
      return parentRoute.id;
    }
    return findRealParentId(parentRoute, routes);
  }
  return route.id === 'root' ? undefined : 'root';
}

export async function loader({ request }: LoaderFunctionArgs) {
  const build = (await (import.meta.env.DEV
    ? // @ts-ignore
      import('../../build/server/index.js')
    : import(/* @vite-ignore */ import.meta.resolve('../../build/server/index.js')))) as ServerBuild;

  for (const key in build.routes) {
    const route = build.routes[key];
    route.parentId = findRealParentId(route, build.routes);
  }

  return generateSitemap(request, build.routes, {
    siteUrl: process.env.SITE_URL,
  });
}

@toddheslin
Copy link

@pchmn well that makes way more sense! hahaha :-)

@blabassi
Copy link

Works fine with Vite doing it this way too:

import { generateSitemap } from '@nasa-gcn/remix-seo'
import { type LoaderFunctionArgs } from '@remix-run/node'
// @ts-ignore
import { routes } from 'virtual:remix/server-build'

export async function loader({ request }: LoaderFunctionArgs) {
  return generateSitemap(request, routes, {
    siteUrl: 'https://yourdomain.tld',
  })
}

@koikar
Copy link

koikar commented Feb 19, 2024

For development the 'virtual:remix/server-build' works great. You do need to restart the dev server whenever making changes to the sitemap, e.g. excluding a route.

@lpsinger
Copy link
Member

I could use some feedback and reviewers for #8 which fixes the double slash issue.

@kiliman
Copy link

kiliman commented Feb 20, 2024

FYI: If you're still having issues related to remix-flat-routes, please comment on this issue kiliman/remix-flat-routes#105

@lpsinger
Copy link
Member

@kiliman, I am pretty sure that this is a bug in remix-seo.

@kiliman
Copy link

kiliman commented Feb 20, 2024

I took at look at remix-seo and I think I've found the issue.

The issue is you should be skipping over any route entries where path is undefined. These are pathless layouts and are not included in the URL.

EDIT: also check for index routes (since they also don't have a path)

        const manifestEntry = routes[id]
        if (!manifestEntry) {
          console.warn(`Could not find a manifest entry for ${id}`)
          return
        }
+        // pathless layouts are not included
+        if (!manifestEntry.path && !manifiestEntry.index) {
+          return
+        }

        let parentId = manifestEntry.parentId
        let parent = parentId ? routes[parentId] : null
        while (parent) {
+          // only add the parent path if it has a path
+          if (parent.path) {
            // the root path is '/', so it messes things up if we add another '/'
            const parentPath = removeTrailingSlash(parent.path)
            path = `${parentPath}/${path}`
+          }
          parentId = parent.parentId
          parent = parentId ? routes[parentId] : null
        }
        if (path.includes(':')) return
        if (id === 'root') return

-        const entry: SitemapEntry = { route: removeTrailingSlash(path) }
+        const entry: SitemapEntry = { route: `/${removeTrailingSlash(path)}` }
        return entry

@kiliman
Copy link

kiliman commented Feb 20, 2024

Given these routes, here is the sitemap.

❯ tree app/routes        
app/routes
├── _auth+
│   ├── _layout.tsx
│   ├── login.tsx
│   └── profile.tsx
├── _blog+
│   └── hello
│       └── route.tsx
├── _index.tsx
├── _marketing+
│   ├── privacy.tsx
│   └── terms.tsx
├── counter.test.tsx
├── counter.tsx
├── resources+
│   └── healthcheck.tsx
└── sitemap[.xml].ts
<Routes>
  <Route file="root.tsx">
    <Route file="routes/_auth+/_layout.tsx">
      <Route path="login" file="routes/_auth+/login.tsx" />
      <Route path="profile" file="routes/_auth+/profile.tsx" />
    </Route>
    <Route path="hello" file="routes/_blog+/hello/route.tsx" />
    <Route index file="routes/_index.tsx" />
    <Route path="privacy" file="routes/_marketing+/privacy.tsx" />
    <Route path="terms" file="routes/_marketing+/terms.tsx" />
    <Route path="counter" file="routes/counter.tsx" />
    <Route path="resources/healthcheck" file="routes/resources+/healthcheck.tsx" />
    <Route path="sitemap.xml" file="routes/sitemap[.xml].ts" />
  </Route>
</Routes>

sitemap

<urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9"
  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://www.sitemaps.org/schemas/sitemap/0.9 http://www.sitemaps.org/schemas/sitemap/0.9/sitemap.xsd">
  <url>
    <loc>https://remix.run/login</loc>
    <priority>0.7</priority>
  </url>
  <url>
    <loc>https://remix.run/profile</loc>
    <priority>0.7</priority>
  </url>
  <url>
    <loc>https://remix.run/hello</loc>
    <priority>0.7</priority>
  </url>
  <url>
    <loc>https://remix.run/</loc>
    <priority>0.7</priority>
  </url>
  <url>
    <loc>https://remix.run/privacy</loc>
    <priority>0.7</priority>
  </url>
  <url>
    <loc>https://remix.run/terms</loc>
    <priority>0.7</priority>
  </url>
  <url>
    <loc>https://remix.run/counter</loc>
    <priority>0.7</priority>
  </url>
</urlset>

@lpsinger
Copy link
Member

@kiliman, would you please move those comments over to #8?

@ben-rogerson
Copy link

@blabassi's example was closest to what I was after, but I made some adjustments so the url doesn’t need to be specified (correct urls in dev too):

import { generateSitemap } from "@nasa-gcn/remix-seo";
import { type LoaderFunctionArgs } from "@remix-run/node";
// @ts-expect-error Virtual modules are not recognized by TypeScript
// eslint-disable-next-line import/no-unresolved
import { routes } from "virtual:remix/server-build";

function getDomainUrl(request: Request) {
  const host =
    request.headers.get("X-Forwarded-Host") ??
    request.headers.get("host") ??
    new URL(request.url).host;
  const protocol = host.includes("localhost") ? "http" : "https";
  return `${protocol}://${host}`;
}

export async function loader({ request }: LoaderFunctionArgs) {
  return generateSitemap(request, routes, {
    siteUrl: getDomainUrl(request),
  });
}

@lpsinger
Copy link
Member

Anyone care to make a PR to capture this in the docs?

@pmbanugo
Copy link

@blabassi's example was closest to what I was after, but I made some adjustments so the url doesn’t need to be specified (correct urls in dev too):

import { generateSitemap } from "@nasa-gcn/remix-seo";
import { type LoaderFunctionArgs } from "@remix-run/node";
// @ts-expect-error Virtual modules are not recognized by TypeScript
// eslint-disable-next-line import/no-unresolved
import { routes } from "virtual:remix/server-build";

function getDomainUrl(request: Request) {
  const host =
    request.headers.get("X-Forwarded-Host") ??
    request.headers.get("host") ??
    new URL(request.url).host;
  const protocol = host.includes("localhost") ? "http" : "https";
  return `${protocol}://${host}`;
}

export async function loader({ request }: LoaderFunctionArgs) {
  return generateSitemap(request, routes, {
    siteUrl: getDomainUrl(request),
  });
}

Thank you!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants