[fix] prerendering not working with kit.paths.base set.

[Karlinator]

Fixes #2230

Local assets were not being excluded from being visited during prerendering because they were not being detected as being one of the local files.

Specifically, this used to work until the semantics of kit.paths.assets was changed in #2189 to only be either empty or an absolute (external) path, and assets were instead prefixed with paths.base if paths.assets was not an absolute path. This line should have been updated then.

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 pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

🦋 Changeset detected

Latest commit: 90349a1

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

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	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

[benmccann]

Did you see there's also another fix pending for this issue at #2318? I'm not that familiar with this code and haven't looked closely at either PR yet to know which makes more sense

[Karlinator]

I have seen that (though somehow only after I had dug through and found this solution). It actually leaves this incorrect behaviour behind and fixes the issue by doing a string replacement before that point instead. That replacement was precisely what the line I fixed is intended to do (and what it did before #2189). Either PR will work, but I suggest that if #2318 is merged it should also remove that pathname.replace(config.kit.paths.assets, '') which does absolutely nothing.

Overall I'd say this seems like a cleaner fix to me.

[baseballyama]

Hi.

I'm author of #2318.
I checked your PR and I think your PR is better than mine.

---

(This is just follow up)

According to current spec, config.kit.paths.assets can be empty string or absolute URL.
So always if (!resolved.startsWith('/') || resolved.startsWith('//')) should be true if config.kit.paths.assets is absolute URL.
So after the process, it doesn't need to consider 'config.kit.paths.assets'.

Therefore my PR's code can move to after the if statement.
And at that time, my PR is almost the same as this PR and this PR has no useless processing compare to mine.
So I think this PR is better than mine.

---

By the way, I have 2 small requests in my opinion.
So if these are reasonable, I'm happy if you will implement them.

1. DRY

Now it has pathname.replace(config.kit.paths.base, '') twice.
So I think decodeURI(parsed.pathname) can be change to decodeURI(parsed.pathname).replace(config.kit.paths.assets, '').

2. Prevent mis-replacement

pathname.replace(config.kit.paths.assets, '') has chance to miss replacement.
For instance, if pathname is aaa/bbb/ccc and config.kit.paths.base is bbb,
pathname.replace(config.kit.paths.assets, '') become aaa//ccc.

So it should use startsWith before replacement.

Thank you!

[Karlinator]

Thank you! I agree on the first point, good spot!

I don't think the startsWith is actually necessary though. By definition, all paths should always start with the base path unless they are external asset paths. External asset paths are already covered by !resolved.startsWith('/') || resolved.startsWith('//') as you mention. String.prototype.replace only replaces the first occurrence. The scenario where it mis-replaces should never happen, and if it does then something else has already gone wrong.

[baseballyama]

Oh right.
That is may not actually be necessary.

Thank you for the discussion!

[sidharthv96]

Just tested this build in my project, the build works perfectly!
I'm able to deploy the built files and they work.

The only issue is that npm run preview is broken when the base path is set. All assets are 404. Seems like #2409 will fix that.

But that is a smaller bug that the build not working, which was keeping mermaid.live on v147 till now.

@benmccann if this can be merged, that blocker would be fixed :)

[Karlinator]

Yup, I had the same sequence of events as you, that's why #2409 came about 😄 Fixed this and then "wait, what's up with preview?"

[benmccann]

thanks for testing it

[nikitaiavdeev]

Great thanks for the fix.

But I still have an issue with service-worker. kit.paths.base doesn't applies as a prefix to 'service-worker.js'. As a result it tries to register it from the root directory and fails.

navigator.serviceWorker.register('/service-worker.js');
instead of
navigator.serviceWorker.register('kit.paths.base/service-worker.js');

[sidharthv96]

@nikitaiavdeev You might have to raise this as a new issue to keep track of it better.