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

feat: Define the MountOptions type #13674

Merged
merged 6 commits into from
Oct 23, 2024
Merged

feat: Define the MountOptions type #13674

merged 6 commits into from
Oct 23, 2024

Conversation

webJose
Copy link
Contributor

@webJose webJose commented Oct 18, 2024

Svelte 5 rewrite

Closes #13655.

Please note that the Svelte codebase is currently being rewritten for Svelte 5. Changes should target Svelte 5, which lives on the default branch (main).

If your PR concerns Svelte 4 (including updates to svelte.dev.docs), please ensure the base branch is svelte-4 and not main.

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

Copy link

changeset-bot bot commented Oct 18, 2024

🦋 Changeset detected

Latest commit: cdfa314

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

This PR includes changesets to release 1 package
Name Type
svelte Minor

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

@webJose webJose changed the title Main feat: Define the MountOptions type Oct 18, 2024
@brunnerh
Copy link
Member

brunnerh commented Oct 18, 2024

This is backwards.

  • Move the MountOptions type to packages/svelte/src/index.d.ts.
  • Add to import in render.js and adjust the two mount function signatures
    - /** @import { Component, ComponentType, SvelteComponent } from '../../index.js' */
    + /** @import { Component, ComponentType, SvelteComponent, MountOptions } from '../../index.js' */
  • Run the build or generate:types script of the package to update the types/index.d.ts.
  • (Probably run format script on the workspace to fix stuff.)

There also seems to be some parentheses missing here:

	intro?: boolean;
- } & {} extends Props ? {
+ } & ({} extends Props ? {
	/**
	 * Component properties.
	 */
	props?: Props;
} : {
	/**
	 * Component properties.
	 */
	props: Props;
- }
+ })

@webJose
Copy link
Contributor Author

webJose commented Oct 18, 2024

I see, so types/index.d.ts is a generated file. I am not used to storing generated files in source control. That was a curve ball for me.

@brunnerh
Copy link
Member

Agreed, it's a bit odd...

@brunnerh
Copy link
Member

You could add a feat: changeset mentioning that the mount options are now exported.

@webJose
Copy link
Contributor Author

webJose commented Oct 18, 2024

Thanks for the hand holding. Appreciated. If I can bother you for one more thing: How is the changeset generated? Never used this before. Any link for a quick tutorial?

@brunnerh
Copy link
Member

The bot posted a link in its comment, also never dealt with them.

@webJose
Copy link
Contributor Author

webJose commented Oct 18, 2024

Shoot! I did not notice. Alright, I think I'm all set then. Cheers!

@webJose
Copy link
Contributor Author

webJose commented Oct 18, 2024

Ok, I think I'm done.

@brunnerh
Copy link
Member

Looks good to my non-maintainer eyes, don't forget to publish this as a proper PR.

@webJose
Copy link
Contributor Author

webJose commented Oct 18, 2024

Meaning taking it out of draft mode?

@webJose webJose marked this pull request as ready for review October 18, 2024 17:09
Copy link
Contributor

@trueadm trueadm left a comment

Choose a reason for hiding this comment

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

Thanks

@trueadm
Copy link
Contributor

trueadm commented Oct 23, 2024

Please could you run pnpm format on your PR to fix the lint issues?

@webJose
Copy link
Contributor Author

webJose commented Oct 23, 2024

Hello. I ran pnpm format, but there must be something wrong going on because once it finishes, it shows over 3.000 changes:

image

Even more interesting, diffing doesn't seem to highlight any changes in any of the files that I checked. Any pointers?

@trueadm
Copy link
Contributor

trueadm commented Oct 23, 2024

@webJose I'll update your PR.

@webJose
Copy link
Contributor Author

webJose commented Oct 23, 2024

Thanks! Very kind of you. Much appreciated.

@trueadm trueadm merged commit 95980d1 into sveltejs:main Oct 23, 2024
7 checks passed
@github-actions github-actions bot mentioned this pull request Oct 23, 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.

Declare type for the options parameter of the mount() function on its own
3 participants