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

[NextjsSite construct] add basePath support back into NextjsSite construct #3613

Merged
merged 5 commits into from
Feb 29, 2024

Conversation

jplwood
Copy link
Contributor

@jplwood jplwood commented Jan 9, 2024

Partially addresses #2964

The original, now-deprecated version of the NextjsSite construct (as far as I can tell, it's deprecated because the non-deprecated construct uses SsrSite as a base, whereas the deprecated one doesn't?) supported basePath(see deprecated code, and Next.js docs on basePath), a crucial feature for hosting apps behind a reverse proxy (and a major convenience when linking around if all of your app's routes are all heavily nested). When the new version of NextjsSite construct was released, basePath support seemed to have been removed.

This PR takes that original functionality and adds it back to the new NextjsSite construct.

Unclear if this is safely additive or if it's a breaking change.

Copy link

changeset-bot bot commented Jan 9, 2024

🦋 Changeset detected

Latest commit: ce73cb0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
sst Patch
@sst/console Patch
create-sst Patch
astro-sst Patch
svelte-kit-sst Patch
solid-start-sst Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment on lines 422 to 427
private pathPattern(pattern: string): string {
const { basePath } = this.routesManifest || {};
return basePath && basePath.length > 0
? `${basePath.slice(1)}/${pattern}`
: pattern;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken from:

private pathPattern(pattern: string): string {
const { basePath } = this.routesManifest || {};
return basePath && basePath.length > 0
? `${basePath.slice(1)}/${pattern}`
: pattern;
}

@jayair
Copy link
Contributor

jayair commented Jan 9, 2024

Thanks! Let me ask the team to review.

@jplwood
Copy link
Contributor Author

jplwood commented Jan 12, 2024

@jayair any updates here? We're nearing the end of our evaluation period of SST and hoping to use this getting released as an example of our ability to influence/improve community implementations (as an alternative to doing things more in-house)

@jayair
Copy link
Contributor

jayair commented Jan 12, 2024

Ah I see. Let me see if I can bump up the timeframe on this. We've been a little behind after the holidays.

@justindra
Copy link
Contributor

justindra commented Jan 30, 2024

Just pulled this down and tested it and it seems to work for my stuff. Thanks @jplwood

@josepardo-eb
Copy link

@jayair any idea about when this could be merged?

@jayair
Copy link
Contributor

jayair commented Feb 15, 2024

Oh no, let me see why it didn't get merged.

@fwang fwang merged commit ab7f847 into sst:master Feb 29, 2024
1 check passed
@github-actions github-actions bot mentioned this pull request Feb 29, 2024
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

Successfully merging this pull request may close these issues.

None yet

5 participants