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

@sveltejs/enhanced-img: Fix client component compilation #11059

Merged
merged 7 commits into from
Nov 18, 2023

Conversation

ottomated
Copy link
Contributor

@ottomated ottomated commented Nov 17, 2023

In JS files, Vite compiles __VITE_ASSET_xxx_ to " + new URL("../path", import.meta.url) + ". This was being inlined directly into the client-side components, meaning the client components were rendering <img src=" + new URL("../path", import.meta.url) + ">. This fixes that.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Copy link

changeset-bot bot commented Nov 17, 2023

🦋 Changeset detected

Latest commit: c1ea90a

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

This PR includes changesets to release 1 package
Name Type
@sveltejs/enhanced-img 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

@ottomated ottomated changed the title fix VITE_ASSET issue @sveltejs/enhanced-img: Fix client component compilation Nov 17, 2023
@benmccann
Copy link
Member

benmccann commented Nov 18, 2023

Thanks for finding and fixing this!

It looks to me like this happens on the client-side JS. I can't reproduce it on the SvelteKit site and I'm guessing that may be because we only do manual imports there. I'm really curious why it doesn't happen with manual imports, but does happen when simply sticking the path in the src. This PR has a small performance hit, so we should investigate a bit to see if we can avoid the import.meta.url stuff from being generated in the first place.

@benmccann
Copy link
Member

@benmccann
Copy link
Member

benmccann commented Nov 18, 2023

Hmm. In #9220 we seem to be forcing relative paths (which was attempted to be changed and then later reverted in #10442). I don't like the API of paths.relative: undefined | false, which I find to be a bit confusing. I don't understand exactly what the undefined case is trying to solve for. It sounds like it might help the Internet Archive? I'd expect the Internet Archive to be able to deal with all the sites out there that are not coded especially for it with a weird half-relative and half-absolute behavior, but maybe that's overly optimistic.

The undefined value causes hydration slowdowns due to different values on the client and server requiring complex comparisons, causes larger bundle sizes from inserting import.meta.url everywhere, and now is causing template creation to be de-optimized. I really dislike it. I'd love to just see a default value of false with undefined removed in Kit 2.0, but am not sure if that's feasible.

We will need a solution like in this PR regardless of whether we can keep Kit in an optimized state because you'll end up with this code when the user sets relative: true. But we should only use a mustache expression when dealing with relative paths. And I do also wonder if we can at least strip the leading "" + and trailing + "" to be slightly more optimized about it in that case.

@ottomated
Copy link
Contributor Author

I also really dislike this solution, but given that the alternative is all images being broken on pages with CSR, might it be better to merge this ASAP and then look into performance improvements?

Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

Agreed. I pushed a couple small changes and filed issues to followup on the rest

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