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

Injected route error in pnpm workspace packages more than one directory deep #7561

Closed
1 task
delucis opened this issue Jul 4, 2023 · 1 comment · Fixed by #7983
Closed
1 task

Injected route error in pnpm workspace packages more than one directory deep #7561

delucis opened this issue Jul 4, 2023 · 1 comment · Fixed by #7983
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority)

Comments

@delucis
Copy link
Member

delucis commented Jul 4, 2023

What version of astro are you using?

2.7.0, 2.7.1, 2.7.2, 2.7.3

Are you using an SSR adapter? If so, which one?

None

What package manager are you using?

pnpm

What operating system are you using?

macOS / Stackblitz

What browser are you using?

Firefox/Chrome/Safari

Describe the Bug

In versions of Astro 2.7.0, 2.7.1, 2.7.2 and 2.7.3 (latest), we’re seeing the starter template failing to build inside the Starlight monorepo (usage via create astro works fine as does astro dev). This seems to be a wider bug with pnpm workspaces.

Minimal reproduction:

  1. Load the StackBlitz repro
  2. It will install dependencies for you
  3. cd sites/docs to move to the package using Starlight
  4. pnpm build to try building the site

This will trigger an error during the generating static routes phase:

Cannot find module '/home/projects/github-kwocty-hrptj2/sites/renderers.mjs' imported from /home/projects/github-kwocty-hrptj2/sites/docs/dist/node_modules/.pnpm/@[email protected][email protected]/node_modules/@astrojs/starlight/404.astro.mjs

For context: 404.astro.mjs is being generated from Starlight’s injected 404 route.

If you move the site up a level to docs/ (instead of sites/docs/) and update pnpm-workspace.yaml accordingly, the project builds fine, so seems to be related to folder depth? Or perhaps something accidentally works for packages one level deep but not for more nested packages?

Also reported here: withastro/starlight#305

What's the expected result?

astro build is able to successfully build the site.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-kwocty-hrptj2

Participation

  • I am willing to submit a pull request for this issue.
@HiDeoo
Copy link
Member

HiDeoo commented Jul 4, 2023

I managed to make the build work by patching the makeAstroPageEntryPointFileName() function.

Just posting what I could find here in case it might help:

  • The routes.find won't return a route for the pageModuleId equals to ../../packages/starlight/404.astro as routeData.route is equal to /404. Only routeData.component seems to be equal to the pageModuleId.
  • This means the function will return pages../../packages/starlight/404.astro.mjs.
  • To build properly, this function needs to return pages/404.astro.mjs instead.

The quick and dirty patch I used to make the build pass is the following, altho it is surely not the best way to fix this as I do not know this part of the codebase well enough and may break other things:

export function makeAstroPageEntryPointFileName(
	prefix: string,
	facadeModuleId: string,
	routes: RouteData[]
) {
	const pageModuleId = facadeModuleId
		.replace(prefix, '')
		.replace(ASTRO_PAGE_EXTENSION_POST_PATTERN, '.');
	let route = routes.find((routeData) => {
-		return routeData.route === pageModuleId;
+		return routeData.route === pageModuleId || routeData.component === pageModuleId;
	});
	let name = pageModuleId;
	if (route) {
		name = route.route;
	}
	if (name.endsWith('/')) name += 'index';
	const fileName = `${name.replaceAll('[', '_').replaceAll(']', '_').replaceAll('...', '---')}.mjs`;
-	if (name.startsWith('..')) {
+	if (pageModuleId.startsWith('..')) {
		return `pages${fileName}`;
	}
	return fileName;
}

@natemoo-re natemoo-re added the - P4: important Violate documented behavior or significantly impacts performance (priority) label Jul 5, 2023
delucis added a commit that referenced this issue Aug 4, 2023
natemoo-re added a commit that referenced this issue Aug 7, 2023
* fix(#7561): refactor astro page filename logic

* chore: add changeset
natemoo-re added a commit that referenced this issue Aug 7, 2023
* fix: inject route! hack!

* refactor: use integration container to resolve all injected routes

* chore: add changeset

* Fix pnpm workspace injectRoute bug

See #7561 (comment)

Closes #7561

* Revert "Fix pnpm workspace injectRoute bug"

This reverts commit 3082c7c.

* Update .changeset/stupid-pants-press.md

Co-authored-by: Chris Swithinbank <[email protected]>

* Update packages/astro/src/vite-plugin-scripts/page-ssr.ts

Co-authored-by: Chris Swithinbank <[email protected]>

* refactor: cleanup injectedRoute resolution logic

---------

Co-authored-by: Chris Swithinbank <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants