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

export VERSION from @sveltejs/kit #9969

Merged
merged 12 commits into from Jun 28, 2023
Merged

Conversation

ivanhofer
Copy link
Contributor

@ivanhofer ivanhofer commented May 18, 2023

Closes #9937

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

@changeset-bot
Copy link

changeset-bot bot commented May 18, 2023

🦋 Changeset detected

Latest commit: 91fcda9

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

.github/workflows/release.yml Outdated Show resolved Hide resolved
packages/kit/types/index.d.ts Outdated Show resolved Hide resolved
packages/kit/scripts/update_version.js Outdated Show resolved Hide resolved
@benmccann
Copy link
Member

@dominikg you're more familiar with the release process. Do you think this will actually work?

@dominikg
Copy link
Member

Is it really needed to parse this at publish and generate an extra module? Can't we just read it from package.json and export?

vite: https://github.com/vitejs/vite/blob/main/packages/vite/src/node/constants.ts#L5

@Rich-Harris
Copy link
Member

Can't we just read it from package.json and export?

Not in the browser

@dominikg
Copy link
Member

Can't we just read it from package.json and export?

Not in the browser

do we need it in the browser though? We are already embedding an application version iirc, so we could embed a kit version too with the same mechanism.

If it's possible to prevent this extra moving part i'd prefer it, even if it was a bit more cumbersome to get the verison in browsers (whats the usecase for that)

@Rich-Harris
Copy link
Member

I mean we can't not allow it in the browser — it would be deeply weird if we couldn't do this sort of thing:

<script>
  import { VERSION } from '@sveltejs/kit';
</script>

<h1>Welcome to SvelteKit version {VERSION}</h1>

We could get it into the browser the same way we populate $app/paths and $env/[type]/public — by including it in the <script> block that initialises the client. But then we'd be sending extra bytes on every render, regardless of whether anything referenced VERSION, and in order to read it we'd have to muck about with a Vite alias that pointed to different modules for server vs client. Seems like quite a bit more complexity to me

@dominikg
Copy link
Member

There is also some complexity to ensure that this generated version doesn't cause problems in the release process.

  • package.json version and version.js version must never be out of sync
  • version.js must be ignored by prettier or any other automations that could cause changes to it
  • what about other packages in this monorepo, if release runs for create-svelte only, should we still write the version? Or add it to all of them

As mentioned above it must work both with and without the github release action.

if all of the above works, cool. Maybe check with changesets if they have a way to update the version.js file together with package.json.

@dominikg
Copy link
Member

it looks like changeset version also does the commit with the updated version already, https://github.com/changesets/changesets/blob/main/packages/cli/src/commands/version/index.ts#L103 so we might have to add a separate commit for updating version.js.

You should be able to run all changeset commands locally to see what they do. the update to version.js would have to be included in the tag for that version, not after.

@dominikg
Copy link
Member

Instead of embedding the kit version into all clients code, applications that need it could put it into data from root layout load 🤔
I do agree that embedding it unconditionally into all clients is not a good way to do it (and could have implications if someone searches for sveltekit applications with vulnerabilities)

@benmccann
Copy link
Member

I do worry this adds quite a bit of complexity to the release process. While it's true that import assertions are still experimental, it seems like inlang could just do an fs.readSync on the file and we could avoid all this. It looks like they only need it on the server-side, so that would be sufficient

@ivanhofer
Copy link
Contributor Author

I never thought that this small change would cause so many problems 😅. Thank you all for your input!

Yes, inlang only needs the version when initializing the vite plugin. So we could indeed use fs.readFile to parse the version info. Can we make sure that the path is always ./node_modules/@sveltejs/kit/package.json? Do all package managers work that way?
What about deno? I recently saw some mentions in the changelog of SvelteKit about deno support. I guess we would need to find another solution for that.

@dominikg
Copy link
Member

Can we make sure that the path is always ./node_modules/@sveltejs/kit/package.json? Do all package managers work that way?

You can use vitefu by @bluwy which has findDepPkgJsonPath to find package.json of a dependency:

import { sveltekit } from '@sveltejs/kit/vite';
import { defineConfig } from 'vite';
import { findDepPkgJsonPath } from 'vitefu';

export default defineConfig(async () => {
	const pkg = await findDepPkgJsonPath('@sveltejs/kit', process.cwd());
	console.log(pkg);
	return {
		plugins: [sveltekit()]
	};
});

If you use this from inside inlang, passing in the right parent path as second argument is needed, within sveltekit process.cwd() should work fine.

@ivanhofer
Copy link
Contributor Author

You should be able to run all changeset commands locally to see what they do. the update to version.js would have to be included in the tag for that version, not after.

Yeah this is not really possible in a non-hacky way (as far as I can tell from reading the docs). The only thing that could work is to use a custom getVersionMessage function, alter the version there and add the file to git's staged changes. This should then work in CI as well as locally.

You can use vitefu by @bluwy which has findDepPkgJsonPath to find package.json of a dependency

Thanks! I guess we will stick to this solution if we don't find a better way to make this PR happen.


Feel free to close the PR if you think it is to much effort to find a reliable solution to this problem.

@dominikg
Copy link
Member

fyi changesets/changesets#1166

@benmccann
Copy link
Member

We ended up adding VERSION to Svelte 4, so it seems reasonable that we could add it here as well. We should probably have the two implementations match each other though. Want to update this PR to match sveltejs/svelte#8668?

@ivanhofer
Copy link
Contributor Author

Sure, I will take a look at this in the coming days

import { VERSION } from './version.js';

// runs the version generation as a side-effect of importing
import '../scripts/generate-version.js';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure where to put this..
There is no build step involved in this repo.
I guess this is needed when running the tests and for publishing pnpm changeset:version should be called. Are there also other places where it is needed to update the version info?

@ivanhofer
Copy link
Contributor Author

@benmccann I have updated this PR. I tried to match the implementation of the svelte repo as close as possible, but there are still minor differences.

@dummdidumm dummdidumm merged commit 316a4af into sveltejs:master Jun 28, 2023
12 of 13 checks passed
@github-actions github-actions bot mentioned this pull request Jun 28, 2023
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.

expose version variable from @sveltejs/kit
6 participants