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

Handle absolute Vite base URL #9700

Merged
merged 5 commits into from
Jul 29, 2024
Merged

Handle absolute Vite base URL #9700

merged 5 commits into from
Jul 29, 2024

Conversation

asmadsen
Copy link
Contributor

@asmadsen asmadsen commented Jul 3, 2024

Currently if you set the base property in vite.config.ts to an full url (ie http://localhost:5173/), when running vite:dev the modulepreload link tags remove the second slash(/) in http://local....

Current behaviour:

<link rel="modulepreload" href="http:/localhost:5173/app/entry.client.tsx" crossorigin="anonymous">

Expected behaviour:

<link rel="modulepreload" href="http://localhost:5173/app/entry.client.tsx" crossorigin="anonymous">

This is not the case when first running vite:build then serving the output with remix-serve ./build/server/index.js, then it behaves as expected.

I also checked the classic remix compiler, it also behaves as expected.

Testing Strategy:

In a fresh remix project, I pointed @remix/run to the local path of this MR using pnpm link.
Making sure the base property inside vite.config.ts was set to the origin of the local dev server (ie http://localhost:5173/)

pnpm create remix my-test
cd my-test
pnpm run dev

Then I verified that the modulepreload link href works as the expected behaviour above.

I'm a bit unsure if all of the places should be changed, however since they all are based on the same path I think this should be correct. And in the limited testing I did there where no issues.

Copy link

changeset-bot bot commented Jul 3, 2024

🦋 Changeset detected

Latest commit: aaecd6c

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

This PR includes changesets to release 16 packages
Name Type
@remix-run/dev Patch
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/css-bundle Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/testing 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 980 to 983
url: new URL(
VirtualModule.url(browserManifestId),
ctx.remixConfig.publicPath
).href,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think ctx.remixConfig.publicPath is guaranteed to be an absolute URL here, is it? If it's not, this will throw an exception:

> node
Welcome to Node.js v20.12.2.
Type ".help" for more information.
> new URL("file.txt", "/public")
Uncaught TypeError: Invalid URL
    at new URL (node:internal/url:796:36) {
  code: 'ERR_INVALID_URL',
  input: 'file.txt',
  base: '/public'
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I've updated the PR to use a function implemented the same way as the way axios combines urls

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jul 3, 2024

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@brophdawg11
Copy link
Contributor

I think @pcattori and/or @markdalgleish should review this since they're more familiar with the vite plugin internals

Copy link
Member

@markdalgleish markdalgleish left a comment

Choose a reason for hiding this comment

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

I've added some tests, but otherwise this looks great to me. Thanks for the PR!

@markdalgleish markdalgleish merged commit ef612a7 into remix-run:dev Jul 29, 2024
9 checks passed
@markdalgleish markdalgleish changed the title fix publicPath joining with module paths to support absolute publicPath Handle absolute Vite base URL Jul 29, 2024
Copy link
Contributor

🤖 Hello there,

We just published version 2.11.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

github-actions bot commented Aug 1, 2024

🤖 Hello there,

We just published version 2.11.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants