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

Version info thingy #8761

Merged
merged 20 commits into from
Jun 22, 2023
Merged

Version info thingy #8761

merged 20 commits into from
Jun 22, 2023

Conversation

gtm-nayan
Copy link
Contributor

Closes #8636

HEADS UP: BIG RESTRUCTURING UNDERWAY

The Svelte repo is currently in the process of heavy restructuring for Svelte 4. After that, work on Svelte 5 will likely change a lot on the compiler aswell. For that reason, please don't open PRs that are large in scope, touch more than a couple of files etc. In other words, bug fixes are fine, but feature PRs will likely not be merged.

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
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • 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 npm test and lint the project with npm run lint

@gtm-nayan gtm-nayan changed the base branch from master to version-4 June 19, 2023 16:18
import { VERSION } from '../shared/version';

// @ts-ignore
if (typeof window !== undefined) (window.__svelte || (window.__svelte = {})).version = VERSION;
Copy link
Member

Choose a reason for hiding this comment

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

should we be paranoid here and don't do anything at all if window.__svelte is already defined? We might override sth else here. Opposite of that: Do we want to support that multiple versions could exist on one through a set? Right now last one wins, with my proposal first one wins. I'm probably in favor of keeping it simple and not do that

Copy link
Member

Choose a reason for hiding this comment

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

Good question. @Rich-Harris had suggested multiple version in an array originally: #8636

Multiple versions is more powerful though I suspect it'd be rarely used

I don't know if there's any difference between first one wins and last one wins

packages/svelte/src/runtime/tag-version.js Outdated Show resolved Hide resolved
packages/svelte/package.json Outdated Show resolved Hide resolved
@dummdidumm dummdidumm added this to the 4.x milestone Jun 20, 2023
Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Needs a changeset, but otherwise looks good to me

@dummdidumm dummdidumm changed the base branch from version-4 to master June 20, 2023 20:45
@benmccann benmccann deleted the branch master June 20, 2023 20:45
@benmccann benmccann closed this Jun 20, 2023
@dummdidumm dummdidumm reopened this Jun 20, 2023
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@@ -134,6 +134,10 @@ export function create_loader(compileOptions, cwd) {
resolved = `${svelte_path}src/runtime/index.js`;
}

if (id === 'svelte/internal/tag-version') {
Copy link
Member

Choose a reason for hiding this comment

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

perhaps this should be called disclose-version to match the option name

@benmccann
Copy link
Member

My personal opinion is that we should expose only the major version. We can already get stats for full versions on https://www.npmjs.com/package/svelte?activeTab=versions, so including the full version doesn't provide much benefit since we don't need it for getting stats about which versions are more used. While hiding the full version doesn't make a site secure, it will add a bit more work for attackers who want to go after vulnerabilities at scale.

@benmccann
Copy link
Member

just FYI @gtm-nayan, it looks like the test failure is a real one

@gtm-nayan
Copy link
Contributor Author

Fixed, thanks for the heads up.

@dummdidumm dummdidumm merged commit 5702142 into master Jun 22, 2023
6 of 7 checks passed
@dummdidumm dummdidumm deleted the version-info-thingy branch June 22, 2023 08:48
This was referenced Jun 22, 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.

Should Svelte/SvelteKit expose which versions are running?
3 participants