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

fix: properly prefix all scripts with base path #2334

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

deer
Copy link
Contributor

@deer deer commented Feb 23, 2024

closes #2326

There was a big misunderstanding on my part -- in particular that a different server might be listening at the root, so the redirect built into context.ts will never happen. The existing test (as mentioned in the issue) passes, but only because no one else is listening, and fresh is able to perform the redirect.

I briefly considered writing a docker container to orchestrate this properly, but then I didn't want to have docker running in order to run these tests. (My laptop is old and bad.) So instead the two new tests very thoroughly assert that scripts included in the html are properly prefixed with the base path. And yes, they fail if the fix is reverted.

As for the fix itself: I just needed to prepend the asset URL with the base path.

Copy link
Collaborator

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

This is great, thanks for fixing this 👍

@marvinhagemeister marvinhagemeister merged commit 8f30313 into denoland:main Feb 23, 2024
7 checks passed
@deer deer deleted the 2326_base_path branch February 23, 2024 22:04
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.

imported revive function and islands from script into browser is not taking basepath into account
2 participants