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: replace version in generateBundle #12700

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

gtm-nayan
Copy link
Contributor

@gtm-nayan gtm-nayan commented Sep 21, 2024

In #8957 one of the desired outcomes was that the framework code would have its own chunk with a stable hash for better caching, but that didn't really happen because the client imports something that references version, through __sveltekit/environment for created_updated_store and version hash through __sveltekit/path, which meant that when the developer changes their app code and increments/changes the version it'd also change the above mentioned modules and subsequently most chunks with the framework code

In this PR the version is injected in generateBundle once the chunk hashes have already been calculated so that just changing the version doesn't affect the hashes for chunks without any actual code changes

fixes #12260


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:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

Copy link

changeset-bot bot commented Sep 21, 2024

🦋 Changeset detected

Latest commit: 7815107

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

SUBSTITUTION_APP_VERSION_HASH,
version_hash.padEnd(SUBSTITUTION_APP_VERSION_HASH.length, ' ')
)
.replaceAll(SUBSTITUTION_APP_VERSION, JSON.stringify(kit.version.name).slice(1, -1));
Copy link
Member

Choose a reason for hiding this comment

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

Any danger of this messing with source map positions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is, tried to mitigate that for the hash with the padding but doing so for the version would change the version string.

Could also match the surrounding quotes and add the padding after those.

Copy link
Member

Choose a reason for hiding this comment

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

Will padding suffice? i.e. will the replacement always be smaller than the placeholder?
(I think matching the surrounding quotes and add padding after is a good idea)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed now I think,
if version is longer than placeholder then the placeholder is now padded with underscores
if version is shorter than placeholder then version is padded with spaces after the quotes

@@ -9,3 +9,6 @@ export const GENERATED_COMMENT = '// this file is generated — do not edit it\n
export const ENDPOINT_METHODS = ['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS', 'HEAD'];

export const PAGE_METHODS = ['GET', 'POST', 'HEAD'];

export const SUBSTITUTION_APP_VERSION_HASH = '__SVELTEKIT_APP_VERSION_HASH__';
Copy link
Member

Choose a reason for hiding this comment

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

These two constants could use a jsdoc comment explaining a bit why we need them (basically a trimmed down version of the PR description)

@gtm-nayan
Copy link
Contributor Author

Dunno why the type check is failing, in my editor it narrows down the type to OutputChunk but the cli doesn't.

@eltigerchino
Copy link
Member

eltigerchino commented Sep 22, 2024

Dunno why the type check is failing, in my editor it narrows down the type to OutputChunk but the cli doesn't.

It happens for me too. Must be Kit's version of TypeScript. If you open the package as root in Code and use the workspace TypeScript version (5.4.5) you'll see the error in the IDE. We should probably bump the dev typescript version to 5.5+

IDE errors

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.

svelte.config.js version changes cause most client chunks to generate a new hash despite no other changes
3 participants