Rebuild types, migrate to vue3, extend rollup config #1542
Conversation
| @@ -0,0 +1,3 @@ | |||
| import IframeResizer from './iframe-resizer.vue' | |||
|
|
|||
| export default IframeResizer | |||
There was a problem hiding this comment.
I would suggest using a default export for the component, so developers can freely choose the name under which they install iframe-resizer.
There was a problem hiding this comment.
I just used the type in packages/react/index.d.ts as a reference
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes the Vue component implementation by migrating from Vue 2 to Vue 3 using the Composition API and improves TypeScript support across packages. The changes involve restructuring the build system to use esbuild and vue-tsc for better type generation.
- Migrates Vue component from Options API to Composition API with TypeScript
- Replaces rollup-plugin-typescript with esbuild for better performance
- Introduces centralized type definitions in packages/core/types.ts
Reviewed Changes
Copilot reviewed 10 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.base.json | Establishes base TypeScript configuration for the project |
| rollup.config.mjs | Updates build pipeline to use esbuild and adds type generation |
| packages/vue/tsconfig.json | Vue-specific TypeScript configuration extending base config |
| packages/vue/index.ts | New TypeScript entry point replacing JavaScript version |
| packages/vue/index.js | Removes old JavaScript entry point |
| packages/vue/iframe-resizer.vue | Migrates to Vue 3 Composition API with TypeScript |
| packages/core/types.ts | Centralizes type definitions for reuse across packages |
| packages/angular/directive.ts | Updates to use shared types from core package |
| package.json | Adds Vue 3 and new build dependencies |
| build/pkgJson.js | Updates package configuration for type declarations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| // Resizer Events | ||
| export type ResizerEvents = { | ||
| onCLosed?: (iframeId: string) => void // Remove in v6 |
There was a problem hiding this comment.
There's a typo in the event name 'onCLosed' - it should be 'onClosed' with a lowercase 'L'.
| onCLosed?: (iframeId: string) => void // Remove in v6 | |
| onClosed?: (iframeId: string) => void // Remove in v6 |
Copilot uses AI. Check for mistakes.
| execSync( | ||
| 'vue-tsc -p packages/vue/tsconfig.json --declaration --emitDeclarationOnly --declarationMap false', | ||
| { stdio: 'inherit' }, | ||
| ) |
There was a problem hiding this comment.
Using execSync for type generation is a blocking operation that could slow down the build process. Consider using the async exec() or integrating vue-tsc programmatically for better performance.
Copilot uses AI. Check for mistakes.
| }) => { | ||
| this.onScroll.next(event) | ||
| return true | ||
| }, |
There was a problem hiding this comment.
The hardcoded 'return true' value should be documented or made configurable. This change affects the scroll behavior but lacks explanation for why it always returns true.
Copilot uses AI. Check for mistakes.
|
Hi, Thanks for this. My long term plan is to refactor the index.js files in core and child packages into smaller more testable components and then migrate to TS. As part of this I'd like to move from Rollup to Vite. When I moved to Rollup 18 months ago, I looked a Vite, but at that time their was no way to get it to create multiple packages. This will then be released as version 6. I agree the types are a bit of a mess, they where contributed by different people and I'm not much of a TS coder. In the meantime my main concern is to avoid any breaking changes. So I'm wondering if in tidying up the types it has created any changes that would break builds for people currently uses this with TS? D. |
|
Hi @davidjbradshaw , I would say that if the JS modules are migrated to TypeScript correctly and the types match those already defined in the Regarding this PR: I tried migrating the React package and noticed that it’s a huge effort to cover everything within this PR. I would suggest reducing the scope to the Vue package only. Once you migrate to TypeScript, I would be happy to support you. Thai |
|
Hey, So sorry for the slow reply, I've been ill in bed all week and I'm still not feeling that great. I've tried to look through this, but my brain is still melting when I look at code. I agree it would be great just to focus on Vue for this PR. That is the biggest miss when it comes to TS support at the moment. Thanks, |
|
Hi, |
|
Thank you. I'm slowly getting a bit better. |
2 participants
Hi @davidjbradshaw,
I don’t have permission to manage the branch divine-master. So I created this PR to address some issues related to the Vue 3 migration and type definitions in this project:
esbuildandvue-tscto compile the Vue components and their corresponding type declarations. Therollup-plugin-vueis no longer well supported - Vite is now the recommended approach. I had to runvue-tscmanually (seerollup.config.mjs) to build the type declarations foriframe-resizer.vue. This should fix the issue mentioned in divine-master.This PR is currently a proposal. Before continuing, I’d like to confirm with you if it’s okay that I:
index.d.ts. For full TypeScript support, I would recommend refactoringindex.jsintoindex.ts(which could be a TODO for the future).If you feel this is too much or conflicts with your current roadmap, I can reduce the scope of this PR to
packages/vueonly.I'm always open to discussion!