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

Implemented support for parsing multiple exports #4

Merged
merged 5 commits into from
Dec 6, 2019

Conversation

Aaron-Pool
Copy link
Contributor

@Aaron-Pool Aaron-Pool commented Dec 6, 2019

Related issues / Problem

vue-styleguidist/vue-styleguidist#665

What I did

Updated vue-docgen-api to 4.1.1, then converted loader to use parseMulti to support multiple exports in a file. Added a test for multiple exports as well.

Also, two of your other tests were operating under the assumption that default exports weren't supported by vue-docgen-api, but I think that's not the case. It seems like it just doesn't support export default when used with Vue.extends({}) (which should maybe be a case @elevatebart should add support for, since I think that's a common method to get correct typescript intellisense?). I fixed those tests and also added cases for mixed exports (default and named). Those changes might be outside of the scope of this PR, so I put them in a separate commit and can move them out, if requested.

Checklist

  • Added one or more test
  • This PR does not includes any other changes that is unrelated to the problem/feature
    - Again, see note about test changes above, I can move that commit out if need be. (I just removed this commit for now, to reduce the noise)

@pocka @shilman @elevatebart

@shilman
Copy link

shilman commented Dec 6, 2019

@Aaron-Pool AWESOME!! Thanks so much for putting this together!

One comment is that the peer dep for vue-docgen-loader should be updated to "^4.1" and it should probably be a major version bump for vue-docgen-loader.

@pocka Would love to get this merged ASAP 🙏🙏🙏 😁

@Aaron-Pool
Copy link
Contributor Author

@shilman Ah, good point on the version number. I'll get that fixed.

For the second point though, why a major version bump? It shouldn't break anyone current usage--unless I misunderstand something. It's only adding additional docgen info to exports that were previously getting none. I thought major version bumps were reserved for incompatible API changes?

@pocka pocka added the enhancement New feature or request label Dec 6, 2019
@pocka
Copy link
Owner

pocka commented Dec 6, 2019

@shilman

One comment is that the peer dep for vue-docgen-loader should be updated to "^4.1" and it should probably be a major version bump for vue-docgen-loader.

Oh, my bad:scream: It should be >=3 to support 3.x and 4.x~.

@shilman
Copy link

shilman commented Dec 6, 2019

@Aaron-Pool major version bump since vue-docgen-api is a peer dep, not a direct dependency, so upgrading vue-docgen-loader also requires upgrading vue-docgen-api manually. I'm actually not sure the semver rules about this, but it wouldn't hurt to do a major version bump.

Certainly the current code here is not compatible with 3.x because parseMulti didn't exist in 3.x...

src/index.js Outdated Show resolved Hide resolved
@Aaron-Pool
Copy link
Contributor Author

Ok, I went ahead and removed the extra tests about export default just to reduce the noise on the PR. I also updated the peer dependency as requested.

@pocka
Copy link
Owner

pocka commented Dec 6, 2019

Published in v1.2.0

For the export default problem, I opened #5

@pocka pocka mentioned this pull request Dec 6, 2019
2 tasks
@Aaron-Pool
Copy link
Contributor Author

@pocka Thanks for the merge. And I honestly have no idea how vue-docgen-api somehow ended up in the regular dependency list, sorry about that 🤔 Also apologies for the linter errors, I was assuming it would reject any commits that had linting issues. Kind of a messy PR on my part, thanks for picking up the slack 🙌

@pocka
Copy link
Owner

pocka commented Dec 6, 2019

@Aaron-Pool
No, problem! and Thank you so much for creating PR 😊 (and using PR template!)

pocka added a commit that referenced this pull request Dec 15, 2019
Previously, the loader injects `component.__docgenInfo = ...` to the
source code regardless of existence of the `component` variable. This
breaks Component files that exports component with default export
statement. This commit changes how the loader inject `__docgenInfo` to
components.

In addition to that, it fixes inconsistent path for `__docgenInfo`.
For SFCs, we can use this:

```js
import Component from 'component.vue'
console.log(Component.__docgenInfo)
```

But for non-SFCs, we need to dig `option` property:

```js
import Component from 'component.js'
console.log(Component.options.__docgenInfo)
```

This commit remove the inconsistency by attaching `__docgenInfo` to the
component directly. You can access `component.__docgenInfo` even for
non-SFCs.

---

The code of default export replacement is written by @Aaron-Pool.
Thanks!

Thread: <#4 (comment)>
Code: <https://astexplorer.net/#/gist/2dfbbbc37e0f53d31bd6f2a1bedca9c0/latest>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants