-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
🦋 Changeset detectedLatest commit: 91fcda9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
@dominikg you're more familiar with the release process. Do you think this will actually work? |
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 |
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) |
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 |
There is also some complexity to ensure that this generated version doesn't cause problems in the release process.
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. |
it looks like 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. |
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 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 |
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 |
You can use vitefu by @bluwy which has 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. |
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
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. |
We ended up adding |
Sure, I will take a look at this in the coming days |
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
e1207fc
to
57f13de
Compare
import { VERSION } from './version.js'; | ||
|
||
// runs the version generation as a side-effect of importing | ||
import '../scripts/generate-version.js'; |
There was a problem hiding this comment.
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?
@benmccann I have updated this PR. I tried to match the implementation of the |
Closes #9937
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.