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: export private legacy ComponentType (#14256) #14257

Merged

Conversation

Theo-Steiner
Copy link
Contributor

@Theo-Steiner Theo-Steiner commented Nov 11, 2024

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 and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

The transitionary interface ComponentType not being exported from the *.svelte module declaration causes issues when running tsc with "declaration": true: error TS4082: Default export of the module has or is using private name 'ComponentType'.

Exporting the interface from the solves this problem in my reproduction (see: #14256)

Fixed Reproduction

pnpm add https://pkg.pr.new/svelte@14257

holy smokes, pkg.pr.new is cool:
Broken reproduction with svelte@5.1.9

Fixed reproduction with svelte@https://pkg.pr.new/svelte@14257

Question:

since packages/svelte/src/ambient.d.ts is included in packages/svelte/types/index.d.ts when building I wasn't sure if I should commit both files into this branch?
the ci lint job told me what to do

Copy link

changeset-bot bot commented Nov 11, 2024

🦋 Changeset detected

Latest commit: c4c544b

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

This PR includes changesets to release 1 package
Name Type
svelte 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

@Rich-Harris
Copy link
Member

preview: https://svelte-dev-git-preview-svelte-14257-svelte.vercel.app/

this is an automated message

@Theo-Steiner Theo-Steiner changed the title fix: export private component type (#14256) fix: export private ComponentType transitionary interface (#14256) Nov 11, 2024
Copy link
Contributor

Playground

pnpm add https://pkg.pr.new/svelte@14257

@dummdidumm
Copy link
Member

This feels like the wrong fix to me, because it means people can now import ComponentType from every .svelte import, if they're not using the TS plugin. There has to be some other way to make it work without exporting it from the *.svelte module.

@Theo-Steiner Theo-Steiner force-pushed the fix/export-private-component-type branch from 17d8d1a to a8728f8 Compare November 20, 2024 12:49
@Theo-Steiner
Copy link
Contributor Author

I've updated the PR to use a different approach, basically we declare a module svelte/transitionary in this case, that exports the TransitionaryComponentType instead. That way typescript is happy and users can't accidentally import the transitionary type

@Rich-Harris
Copy link
Member

Can we use 'legacy' rather than 'transitionary'?

@Rich-Harris
Copy link
Member

(my spellchecker doesn't even recognize the latter as a real word)

@Theo-Steiner
Copy link
Contributor Author

Theo-Steiner commented Nov 20, 2024

Can we use 'legacy' rather than 'transitionary'?

I agree that transitionary is a bit weird 😅
svelte/legacy already exists though and I wasn't sure if redeclaring it would cause any problems since they end up in the same svelte/types/index.d.ts module in the end

@dummdidumm
Copy link
Member

dummdidumm commented Nov 20, 2024

It will definetely cause problems. But I suggest instead to not create a new module for it, rather export a type from svelte/legacy which does what you want and use that within the svelte module declaration

@Theo-Steiner
Copy link
Contributor Author

rather export a type from svelte/legacy which does what you want and use that within the svelte module declaration

I moved the type to packages/svelte/src/legacy/legacy-client.js so that is exported as part of 'svelte/legacy' & renamed it to LegacyComponentType, is this along the lines of what you envisioned?

@dummdidumm
Copy link
Member

looks good - add a changeset and we're good to merge 👍

@Theo-Steiner Theo-Steiner changed the title fix: export private ComponentType transitionary interface (#14256) fix: export private legacy ComponentType (#14256) Nov 22, 2024
@Theo-Steiner
Copy link
Contributor Author

Added a changeset 🎉

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.

thank you!

@dummdidumm dummdidumm merged commit e721d96 into sveltejs:main Nov 22, 2024
10 checks passed
@github-actions github-actions bot mentioned this pull request Nov 22, 2024
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.

3 participants