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 options type for options.floatingUIOptions #3011

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

patrikholcak
Copy link
Contributor

About

Shepherd is already using ComputePositionConfig internally in a few places, but the options object itself is typed as object, which isn’t really helpful. Not sure if this is done because of some problem with docs — feel free to close this PR in that case!

Proposed changes

  • add correct type to Step.floatingUIOptions

Copy link

vercel bot commented Oct 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
shepherd-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 15, 2024 7:19pm
shepherd-landing ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 15, 2024 7:19pm

Copy link

vercel bot commented Oct 11, 2024

@patrikholcak is attempting to deploy a commit to the shipshapecode Team on Vercel.

A member of the Team first needs to authorize it.

@RobbieTheWagner
Copy link
Member

@patrikholcak hitting this error:

17:38:26 [ERROR] [starlight-typedoc-plugin] node_modules/shepherd.js/src/utils/floating-ui.ts:63:17 - error TS2742: The inferred type of 'mergeTooltipConfig' cannot be named without a reference to '.pnpm/@floating-ui+core@1.6.5/node_modules/@floating-ui/core'. This is likely not portable. A type annotation is necessary.

@vercel vercel bot temporarily deployed to Preview – shepherd-landing October 11, 2024 18:34 Inactive
@patrikholcak
Copy link
Contributor Author

Yeah, that explains why it hasn’t been added in the first place 😄 I don’t like doing it, but would you be ok with me copying some of the declarations from floating ui (bad, I know) or should I look into the docs plugin? Maybe there’s an option to ignore a type or make it "external"? 🤷

I probably won’t have time over the weekend though

Due to the way pnpm installs modules and how typescript declarations resolving works, typescript couldn’t be sure what the return type is. We can either specify the return value or cast (less safe)
@patrikholcak
Copy link
Contributor Author

patrikholcak commented Oct 15, 2024

@RobbieTheWagner it should be fixed now (works on my machine!) let me know if it does and I’ll either cherry-pick this into #3012 or wait for this to get merged and then just rebase/merge these changes into #3012

edit: The problem was due to how pnpm saves node modules and how typescript type resolving works (as far as I understand the issue)

Here’s more info microsoft/TypeScript#58176 (comment)

@chuckcarpenter chuckcarpenter merged commit c5bf7bc into shipshapecode:main Oct 15, 2024
9 checks passed
@github-actions github-actions bot mentioned this pull request Oct 15, 2024
@patrikholcak patrikholcak deleted the fix/floating-ui-type branch October 16, 2024 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants