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.

Sign up for GitHub

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

Jump to bottom

New type for IPopover2SharedProps.renderTarget creates ambiguous type where properties are resolved as unknown #5889

Closed
dlech opened this issue Jan 24, 2023 · 4 comments · Fixed by #5907
Closed

New type for IPopover2SharedProps.renderTarget creates ambiguous type where properties are resolved as unknown #5889

dlech opened this issue Jan 24, 2023 · 4 comments · Fixed by #5907
Labels
Domain: API Package: popover2 Status: needs fix verification Type: bug

Comments

@dlech
Copy link
Contributor

dlech commented Jan 24, 2023 •
edited
Loading

8d793f6#r97915117

FYI, this is a breaking change as it creates an ambiguous type. For example, since onFocus is only defined in Popover2HoverTargetHandlers and not Popover2ClickTargetHandlers, TypeScript interprets the the type of onFoucs as unknown.

It causes CI to fail like this: https://github.com/pybricks/pybricks-code/actions/runs/3993794470/jobs/6850831641#step:5:6

Source code that triggered this is here: https://github.com/pybricks/pybricks-code/blob/939ded73fa18e26ae8aeedc88d95f1c9077eccce/src/toolbar/ActionButton.tsx#L103

Environment

  • Package version(s):"@blueprintjs/popover2": "^1.12.0",
  • Operating System: any
  • Browser name and version: any

Code Sandbox

Don't have time to do this at the moment, but see link above for CI failure.

Steps to reproduce

  1. use Tooltip2 with renderTarget as recommended in the docs: https://blueprintjs.com/docs/#popover2-package/tooltip2
  2. update @blueprintjs/popover2 to v1.12.0
  3. Typescript type checking breaks and code no longer compiles

Actual behavior

Expected behavior

Possible solution

Types that inherit from IPopover2SharedProps should Omit<..., 'renderTarget'> and supply the correct type.
dlech referenced this issue Jan 24, 2023
@bvandercar-vt @adidahiya
[popover2] feat: new prop targetProps (#5802)
8d793f6 
Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
Co-authored-by: Adi Dahiya <adahiya@palantir.com>
dlech added a commit to pybricks/pybricks-code that referenced this issue Jan 24, 2023
@dlech
fix breaking change in blueprintjs popover2
7aec8fb 
work around palantir/blueprint#5889
github-actions bot pushed a commit to pybricks/pybricks-code that referenced this issue Jan 24, 2023
@dependabot @dlech
build(deps): bump @blueprintjs/popover2 from 1.11.3 to 1.12.0 (#1454)
7b0dc47 
* build(deps): bump @blueprintjs/popover2 from 1.11.3 to 1.12.0

Bumps [@blueprintjs/popover2](https://github.com/palantir/blueprint/tree/HEAD/packages/core) from 1.11.3 to 1.12.0.
- [Release notes](https://github.com/palantir/blueprint/releases)
- [Changelog](https://github.com/palantir/blueprint/blob/develop/CHANGELOG.md)
- [Commits](https://github.com/palantir/blueprint/commits/@blueprintjs/popover2@1.12.0/packages/core)

---
updated-dependencies:
- dependency-name: "@blueprintjs/popover2"
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* fix breaking change in blueprintjs popover2

work around palantir/blueprint#5889

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: David Lechner <david@pybricks.com>
@adidahiya
Copy link
Contributor

Acknowledged, I noticed this too. I'll sort out a fix, or add guidance about the breaking change this week.

@ZeRego ZeRego mentioned this issue Jan 30, 2023
Merged
@adidahiya adidahiya mentioned this issue Jan 30, 2023
Merged
2 tasks
@adidahiya
Copy link
Contributor

@dlech @ZeRego I believe I've fixed this problem with #5907. Please test out those type changes when they release today/tomorrow.

@adidahiya
Copy link
Contributor

@dlech @ZeRego I've released @blueprintjs/popover2 v1.12.1 with the fix. It seems to have fixed some compilation issues in our internal codebases. Let me know if you still experience issues with this release.

dlech added a commit to pybricks/pybricks-code that referenced this issue Feb 1, 2023
@dlech
toolbar/ActionButton: drop workaround for palantir/blueprint#5889
fae7063 
This was fixed in @blueprintjs/popover2 v1.12.1
@dlech dlech mentioned this issue Feb 1, 2023
Merged
@dlech
Copy link
Contributor Author

dlech commented Feb 1, 2023 •
edited
Loading

It fixes the issue in our case, thanks!

dlech added a commit to pybricks/pybricks-code that referenced this issue Feb 1, 2023
@dlech
toolbar/ActionButton: drop workaround for palantir/blueprint#5889
783da7d 
This was fixed in @blueprintjs/popover2 v1.12.1
dlech added a commit to pybricks/pybricks-code that referenced this issue Feb 1, 2023
@dlech
toolbar/ActionButton: drop workaround for palantir/blueprint#5889
876f1f2 
This was fixed in @blueprintjs/popover2 v1.12.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Domain: API Package: popover2 Status: needs fix verification Type: bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[popover2] fix(Popover2): renderTarget typedef regression palantir/blueprint
2 participants
@adidahiya @dlech