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(use-resize-observer): resolve type conflict #100

Merged
merged 5 commits into from Feb 26, 2022

Conversation

johnsonsu
Copy link
Contributor

@johnsonsu johnsonsu commented Jan 5, 2022

fix typescript 4.2.3 conflict by replacing resize-observer-polyfill with @juggle/resize-observer

Discussion about the type conflict can be found here: https://github.com/que-etc/resize-observer-polyfill/issues/83.\

All existing tests are passing. No new test added.
Closes #101.

@@ -1,6 +1,6 @@
import { ResizeObserver } from "@juggle/resize-observer";
Copy link
Owner

Choose a reason for hiding this comment

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

Juggle's ResizeObserver is a ponyfill so preferably we'd use the one on the window object when it's available

fix typescript 4.2.3 conflict by replacing resize-observer-polyfill with @juggle/resize-observer
Copy link
Owner

@jaredLunde jaredLunde left a comment

Choose a reason for hiding this comment

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

One last thing for SSR then we're good. Thanks! Sorry this took so long.

src/use-resize-observer.ts Outdated Show resolved Hide resolved
@johnsonsu
Copy link
Contributor Author

One last thing for SSR then we're good. Thanks! Sorry this took so long.

All good! Thank you for this awesome project!

@johnsonsu
Copy link
Contributor Author

I might be mistaken but I think your edit deleted the variable assignment. I've added it back.

@jaredLunde
Copy link
Owner

whoops!

@jaredLunde
Copy link
Owner

I think your updated pnpm-lock.yaml changed some dependency versions (partially my fault because the package.json still says latest) so linting is failing.

@johnsonsu
Copy link
Contributor Author

Do you have any idea how to fix this lint error? Lint is passing locally but not the Github Action.

@joxxoxo
Copy link

joxxoxo commented Feb 23, 2022

I've tried to run lint locally and it fails with the same errors:

docker run --rm -it -v "$PWD:/app" -w '/app' node:16-slim bash -c "apt update -qq && apt install -y curl && curl -f https://get.pnpm.io/v6.16.js | node - add --global pnpm && pnpm install && pnpm lint"

PS on main repo it's passing

@joxxoxo
Copy link

joxxoxo commented Feb 23, 2022

Found out that you need to update test, which still references old package and it'll fix linter
https://github.com/johnsonsu/masonic/blob/6ddd1ac6c6872e2c646513f3eff3bf1590f7d6a7/src/use-resize-observer.test.ts#L1

@johnsonsu
Copy link
Contributor Author

Thank you for looking into it @joxxoxo. I've updated those files. Pending @jaredLunde to trigger the workflow.

@joxxoxo
Copy link

joxxoxo commented Feb 24, 2022

@johnsonsu awesome, looking forward to have TS passing on my project 😀
Have you checked tests locally? I think it may need some adjustments to pass

@johnsonsu
Copy link
Contributor Author

All tests are passing locally for me 🤞

@jaredLunde jaredLunde merged commit 38b80bc into jaredLunde:main Feb 26, 2022
github-actions bot pushed a commit that referenced this pull request Feb 26, 2022
## [3.6.3](v3.6.2...v3.6.3) (2022-02-26)

### Bug Fixes

* **use-resize-observer:** resolve type conflict ([#100](#100)) ([38b80bc](38b80bc))
@github-actions
Copy link

🎉 This PR is included in version 3.6.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jaredLunde
Copy link
Owner

Thanks for putting this together! I've been super busy lately and I very much appreciate the community effort.

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.

Typescript type conflict in dependency resize-observer-polyfill
3 participants