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

types(runtime-core): enable plugin option types #3969

Merged
merged 7 commits into from
Nov 14, 2022

Conversation

tony19
Copy link
Contributor

@tony19 tony19 commented Jun 17, 2021

This enables type inference for:

  1. The options arg of a plugin's install function
  2. The options passed to createApp().use()
/**
 * Plugin
 */
type PluginAOptionType = {
  option1?: string;
  option2: number;
  option3: boolean;
}

const PluginA = {
  install(app: App, ...options: PluginAOptionType[]) {

  }
}

const PluginB = {
  install(app: App) {

  }
}

/**
 * Main
 */
createApp({})
  .use(PluginA, { option3: true }) // ❌ option2 (required) missing
  .use(PluginA, {}) // ❌ option2 and option3 (required) missing
  .use(PluginA) // ✅
  .use(PluginA, { option2: 1, option3: true }) // ✅
  .use(PluginA, { option1: 'foo', option2: 1, option3: true }) // ✅
  
  .use(PluginB) // ✅
  .use(PluginB, {}) // ❌ unexpected plugin option

demo

@tony19 tony19 force-pushed the feat/types/plugin-options branch from c35db09 to 41ea5df Compare June 17, 2021 05:58
@posva

This comment was marked as outdated.

@tony19

This comment was marked as outdated.

@antfu
Copy link
Member

antfu commented Oct 3, 2022

@tony19 Can you help resolve the conflicts? Thanks

This enables type inference for:

 1. The options arg of a plugin's install function
 2. The options passed to createApp().use()
@tony19

This comment was marked as resolved.

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

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

I think we should also handle the case where a plugin options are optional and it can be used in both scenarios: with the option provided and without them

test-dts/appUse.test-d.ts Outdated Show resolved Hide resolved
@tony19

This comment was marked as outdated.

tony19 and others added 2 commits October 8, 2022 22:39
Co-authored-by: Eduardo San Martin Morote <posva@users.noreply.github.com>
@antfu antfu requested a review from posva October 10, 2022 04:33
test-dts/appUse.test-d.ts Outdated Show resolved Hide resolved
test-dts/appUse.test-d.ts Outdated Show resolved Hide resolved
@posva
Copy link
Member

posva commented Oct 10, 2022

You were right! but it also should be stricter 😄
I’m reviewing on the phone so I can’t do much code but I will give it a proper review with help later

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

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

I added stricter tests and refactored the options to make it work at tony19-contrib#1

Co-authored-by: Eduardo San Martin Morote <posva@users.noreply.github.com>
@yyx990803 yyx990803 merged commit c513126 into vuejs:main Nov 14, 2022
chrislone pushed a commit to chrislone/core that referenced this pull request Feb 4, 2023
zhangzhonghe pushed a commit to zhangzhonghe/core that referenced this pull request Apr 12, 2023
github-merge-queue bot pushed a commit to ionic-team/ionic-framework that referenced this pull request Jul 16, 2024
Issue number: Internal

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

In v3.2.46 [vue was updated](vuejs/core#3969) to
add support for a generic type argument to the `Plugin` type. In
#29637 Ionic's Vue
plugin was updated to use the generic, but the`@ionic/vue` project is
currently installing v3.2.37.

This results in a local type checking error and build error:
```
(!) Plugin typescript: @rollup/plugin-typescript TS2315: Type 'Plugin_2' is not generic.
src/ionic-vue.ts: (24:24)

24 export const IonicVue: Plugin<[IonicConfig?]> = {
```

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Bumps and pins the dev dependency of `vue` to `3.2.46`. There are
other breaking changes that Vue has shipped in minor/patch cycles that
prevents from updating to the latest.
- Resolves the type checking error locally during build of
the`@ionic/vue` package

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!--
  If this introduces a breaking change:
1. Describe the impact and migration path for existing applications
below.
  2. Update the BREAKING.md file with the breaking change.
3. Add "BREAKING CHANGE: [...]" to the commit description when merging.
See
https://github.com/ionic-team/ionic-framework/blob/main/docs/CONTRIBUTING.md#footer
for more information.
-->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

Validation steps:
1. Checkout the branch
2. Build `core/`
3. In `packages/vue`, install latest pinned dependencies with `npm ci`
4. Sync the changes to the `vue` package with `npm run sync`
5. Open `/packages/vue/src/ionic-vue.ts`
6. Observe: No type errors on L24
7. Run `npm run build`
8. Observe: No build errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants