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

Node middleware adapter doesn't match routes when base is set #6335

Closed
1 task
mdixon4 opened this issue Feb 23, 2023 · 8 comments · Fixed by #7384
Closed
1 task

Node middleware adapter doesn't match routes when base is set #6335

mdixon4 opened this issue Feb 23, 2023 · 8 comments · Fixed by #7384
Assignees
Labels
- P1: chore Doesn't change code behavior (priority)

Comments

@mdixon4
Copy link

mdixon4 commented Feb 23, 2023

What version of astro are you using?

2.0.6

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

Node

What package manager are you using?

pnpm

What operating system are you using?

Windows/WSL/Ubuntu

Describe the Bug

Enable SSR using output: server and using the node adapter with mode: 'middleware', and set a base in astro.config:

import { defineConfig } from 'astro/config'

// https://astro.build/config
import node from '@astrojs/node'

// https://astro.build/config
export default defineConfig({
  output: 'server',
  adapter: node({
    mode: 'middleware',
  }),
  base: '/mybasepath',
})

Create a server.js file that creates an express app and hosts the built middleware at /mybasepath:

import express from 'express'
import { handler } from './dist/server/entry.mjs'

const app = express()

app.use('/mybasepath', express.static('./dist/client'))
app.use('/mybasepath', handler)

app.listen(3000, () => {
  console.log('Listening on http://localhost:3000')
})

Build the site, and run the server using npm run build && node server.js

A 404 status is returned when requesting http://localhost:3000/mybasepath/. When running in dev or preview modes, the site is displayed appropriately.

Inspecting the running code, it looks like match is prepending an additional '/' before matching, so it is trying to find // rather than /.

Link to Minimal Reproducible Example

https://github.com/mdixon4/intergalactic-ice

Participation

  • I am willing to submit a pull request for this issue.
@matthewp matthewp added the - P4: important Violate documented behavior or significantly impacts performance (priority) label Feb 23, 2023
@JerryWu1234
Copy link
Contributor

mark

@JerryWu1234
Copy link
Contributor

JerryWu1234 commented Mar 6, 2023

You needn't add a /mybasepath in the server file and try it out late.

@JerryWu1234
Copy link
Contributor

import express from 'express'
import { handler } from './dist/server/entry.mjs'

const app = express()

app.use('/mybasepath', express.static('./dist/client'))
app.use(handler)

app.listen(3000, () => {
  console.log('Listening on http://localhost:3000')
})

@JerryWu1234
Copy link
Contributor

Because the handle function will handle baseUrl, and the express.static not

I think we can add more descriptions of the middleWare in the doc.
@matthewp
@bluwy
cc

@mdixon4
Copy link
Author

mdixon4 commented Mar 7, 2023

Thank you @wulinsheng123 I can confirm that your solution works. Thanks for investigating.

@JerryWu1234
Copy link
Contributor

I think we also need to update this in the doc.

@matthewp matthewp added - P1: chore Doesn't change code behavior (priority) and removed - P4: important Violate documented behavior or significantly impacts performance (priority) labels Mar 7, 2023
@matthewp
Copy link
Contributor

matthewp commented Mar 7, 2023

Yeah, I suppose mentioning this in the readme could be helpful to people.

@ematipico
Copy link
Member

I'll take this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P1: chore Doesn't change code behavior (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants