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(types): async Component types #11906

Merged
merged 10 commits into from Jun 3, 2021
Merged

fix(types): async Component types #11906

merged 10 commits into from Jun 3, 2021

Conversation

mathe42
Copy link
Contributor

@mathe42 mathe42 commented Feb 11, 2021

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No
  • Not sure if this is considered a breaking change

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

Other information:
Look at this codeexample:

import { AsyncComponent, Component } from 'vue'
const a: AsyncComponent = () => ({
  component: new Promise<Component>((res, rej) => {
    res({})
  }),
})

const b: AsyncComponent = () => ({
  component: () =>
    new Promise<Component>((res, rej) => {
      res({})
    }),
})

The first example is like in the docs https://vuejs.org/v2/guide/components-dynamic-async.html#Handling-Loading-State and the second example should not work (I also looked at the implementation and I think this would not work).

The types are currently like such that the first example throws a type Error and the second throws no error. This PR fixes that.

@posva posva changed the title fix async Component types fix(types): async Component types Feb 11, 2021
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.

Can you add a test?

@mathe42
Copy link
Contributor Author

mathe42 commented Feb 11, 2021

Can you add a test?

What exactly should be tested? - As I understand the repro there are no type tests. I just found https://github.com/vuejs/vue/blob/dev/test/unit/features/component/component-async.spec.js#L322

I can add there a test that the second example doesn't work (like https://codepen.io/mathe42/pen/poNNNYx with check that 'component resolved' is not found in document)
there are tests that the first example works.

@posva
Copy link
Member

posva commented Feb 12, 2021

In this case a type test case would suffice: https://github.com/vuejs/vue/tree/dev/types/test

@mathe42
Copy link
Contributor Author

mathe42 commented Feb 12, 2021

Thanks didn't saw that. Just pushed some tests (also for the simpe usages of AsyncComponent).

Not shure about my const b test. This tests that the syntax that currently is allowed (by the types) failes with this PR.

@mathe42 mathe42 requested a review from posva February 12, 2021 09:24
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.

Thanks!

types/options.d.ts Outdated Show resolved Hide resolved
types/options.d.ts Outdated Show resolved Hide resolved
types/test/async-component-test.ts Outdated Show resolved Hide resolved
@posva posva added the bug label Feb 19, 2021
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.

Can you leave all unrelated changes out of the PR please? I think we will handle the formatting in a different commit

@mathe42
Copy link
Contributor Author

mathe42 commented Feb 19, 2021

Yes sorry, there was more in the commits than I wanted. Working on it.

@mathe42
Copy link
Contributor Author

mathe42 commented Feb 19, 2021

@posva Removed the styling changes.

Hope this is fine now.

@mathe42 mathe42 requested a review from posva February 19, 2021 14:55
types/test/async-component-test.ts Outdated Show resolved Hide resolved
types/test/async-component-test.ts Outdated Show resolved Hide resolved
@posva posva added this to In Review in 2.6.13 Feb 24, 2021
@mathe42
Copy link
Contributor Author

mathe42 commented Feb 25, 2021

I repleced the Vue.component tests with a single one to test if it accepts AsyncComponents...

@mathe42 mathe42 requested a review from posva March 11, 2021 14:41
@posva posva linked an issue Mar 31, 2021 that may be closed by this pull request
@posva posva added this to Planned in 2.6.15 via automation Jun 1, 2021
@posva posva moved this from Planned to In Review in 2.6.15 Jun 1, 2021
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.

Thanks!

@posva posva merged commit c52427b into vuejs:dev Jun 3, 2021
2.6.15 automation moved this from In Review to Done Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2.6.13
In Review
Development

Successfully merging this pull request may close these issues.

Incorrect type for AsyncComponentFactory
2 participants