Skip to content

docs: comments on reactivity functions (fixes #4832) #4836

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

Merged
merged 18 commits into from
Mar 31, 2023
Merged

docs: comments on reactivity functions (fixes #4832) #4836

merged 18 commits into from
Mar 31, 2023

Conversation

defaude
Copy link
Contributor

@defaude defaude commented Oct 22, 2021

Not done yet. This is merely a starter so that others can check if I'm going in the right direction.

Closes #4832

@defaude
Copy link
Contributor Author

defaude commented Oct 22, 2021

I found that there's no documentation for proxyRefs - neither inline nor on the docs page. This function is (only) used in vue-core's component.ts. Is this intented to be "private" / "internal use only" in component.ts? Then maybe we could move it there, as well? If, however, it's supposed to be a common utility, there should be some doc, right?

@defaude
Copy link
Contributor Author

defaude commented Oct 22, 2021

Also: Maybe we could think about having all API-describing documentation in the actual source code and vue-docs somehow pulls those chapters from there? That way, we wouldn't need to maintain the same doc twice.

@defaude
Copy link
Contributor Author

defaude commented Oct 22, 2021

Allow me to summon @yyx990803 to ask for his opinion on this :)

@posva posva changed the title Add documentation for reactivity functions docs: comments on reactivity functions Oct 22, 2021
@posva
Copy link
Member

posva commented Oct 22, 2021

This is a nice start but comments should follow https://tsdoc.org/ and use @example, @param and other directives.

I also think adding full examples for all the methods is too much and won't be as practical when hovering definitions. It would be better to keep a short explanation about the functions with links to the actual docs. And maybe add some short code samples, especially for reactivity functions that are not used that often.

@defaude
Copy link
Contributor Author

defaude commented Oct 22, 2021

If we start adding @param tags - what about internal stuff like e.g. computed's debugOptions param?

And: you're leaning towards having a dedicated, exhaustive documentation in vue-docs and a shorter, more brief version in the in-line docs, right?

@defaude
Copy link
Contributor Author

defaude commented Oct 22, 2021

I've shortened the doc blocks a bit and sprinkled some tsdoc goodness, as well. Did not add @returns, though, as this basically just repeats the first sentence (in a way).

For the dev-only params like debugOptions: I've added an empty @param tag and this looks nice in intelliJ (doesn't show up at all).

But in VSCode, this doesn't work: It shows up in the quick doc overlay... Since most devs here are most likely using VSCode, I removed the empty @param tags now (even though intelliJ will highlight this as a missing param doc declaration for me 😅)

@defaude defaude marked this pull request as ready for review November 7, 2021 16:56
@defaude
Copy link
Contributor Author

defaude commented Nov 7, 2021

@posva I hope I've covered all functions that are exported from the reactivity module. What do you think?

Regarding the documentation for proxyRefs in ref.ts, I'm not 100% sure: #4836 (comment)

@yyx990803
Copy link
Member

Thanks for the great effort! It will probably take me some time to go over all of them but looking good so far.

@netlify
Copy link

netlify bot commented Feb 15, 2022

Deploy Preview for vuejs-coverage ready!

Name Link
🔨 Latest commit 73e035c161e85dba0a3a67e5044e86e06575febd
🔍 Latest deploy log https://app.netlify.com/sites/vuejs-coverage/deploys/6262504b565b640008863c67
😎 Deploy Preview https://deploy-preview-4836--vuejs-coverage.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Feb 15, 2022

Deploy Preview for vue-next-template-explorer ready!

Name Link
🔨 Latest commit 73e035c161e85dba0a3a67e5044e86e06575febd
🔍 Latest deploy log https://app.netlify.com/sites/vue-next-template-explorer/deploys/6262504b19c4ee0008b45f00
😎 Deploy Preview https://deploy-preview-4836--vue-next-template-explorer.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Feb 15, 2022

👷 Deploy Preview for vue-sfc-playground processing.

Name Link
🔨 Latest commit 73e035c161e85dba0a3a67e5044e86e06575febd
🔍 Latest deploy log https://app.netlify.com/sites/vue-sfc-playground/deploys/6262504b66934c0008c4a63c

@defaude
Copy link
Contributor Author

defaude commented Feb 15, 2022

I was quite occupied with my normal job so I missed that this PR was out of sync. It's updated now so it could be merged again 😇

Fixes #4832

@defaude defaude changed the title docs: comments on reactivity functions docs: comments on reactivity functions (fixes #4832) Feb 15, 2022
@defaude
Copy link
Contributor Author

defaude commented Apr 22, 2022

Trying to keep this up to date with the main branch here :)

@EastSun5566
Copy link

I truly believe that those comments can greatly improve DX! I hope that the Vue team can merge them soon 🙏

@defaude
Copy link
Contributor Author

defaude commented Jan 25, 2023

A belated merry christmas and happy new year to everyone 😘 I've updated the PR so it can be merged again.

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.

Thank you for this! I think it's worth getting multiple pairs of eyes on this PR. I think it will be very welcomed! 😄

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.

It's looking great but we should definitely address the unresolved reviews as well!
I think that in general, using the vuejs.org API versions (sometimes without the example or with a shorter one) would make up for an overrall more correct, concise, and complete version.

I added a few comments to the remaining comments but I noticed there were more than I expected so I stopped midway 😅

Comment on lines 131 to 135
* Provides access to the current effect scope.
*
* This may be undefined at times when there's no effect active effect scope at
* this very moment.
Copy link
Member

Choose a reason for hiding this comment

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

I think we should just align with the API docs on this one: https://vuejs.org/api/reactivity-advanced.html#getcurrentscope (much shorter and complete)

@@ -65,26 +65,33 @@ function getTargetType(value: Target) {
export type UnwrapNestedRefs<T> = T extends Ref ? T : UnwrapRefSimple<T>

/**
* Creates a reactive copy of the original object.
* Creates a deeply-reactive copy of the original object.
Copy link
Member

Choose a reason for hiding this comment

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

This still remains

* Return a shallowly-reactive copy of the original object, where only the root
* level properties are reactive. It also does not auto-unwrap refs (even at the
* root level).
* Creates a shallowly-reactive copy of the original object.
Copy link
Member

Choose a reason for hiding this comment

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

still remains

@@ -147,8 +164,32 @@ export type DeepReadonly<T> = T extends Builtin
: Readonly<T>

/**
* Creates a readonly copy of the original object. Note the returned copy is not
* made reactive, but `readonly` can be called on an already reactive object.
* Creates a readonly copy of the original object.
Copy link
Member

Choose a reason for hiding this comment

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

still remains

@defaude
Copy link
Contributor Author

defaude commented Feb 18, 2023

Hey @posva I've shortened the doc blocks a bit, added links to the appropriate docs page where appropriate. What do you think?

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! Almost finished 🙌

@@ -65,26 +65,33 @@ function getTargetType(value: Target) {
export type UnwrapNestedRefs<T> = T extends Ref ? T : UnwrapRefSimple<T>

/**
* Creates a reactive copy of the original object.
* Creates a deeply-reactive copy of the original object.
Copy link
Member

Choose a reason for hiding this comment

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

This one hasn't change: it's not a copy

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 a lot!

@defaude
Copy link
Contributor Author

defaude commented Mar 9, 2023

Hey @posva 👋 Anything else I should do or could/should this be merged now? 🦄

@yyx990803
Copy link
Member

@defaude Thanks for your hard work and patience! This is great.

@yyx990803 yyx990803 merged commit c346af2 into vuejs:main Mar 31, 2023
@defaude defaude deleted the feat/4832-add-docs-for-reactivity-functions branch March 31, 2023 14:11
IAmSSH pushed a commit to IAmSSH/core that referenced this pull request May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add documentation for reactivity functions
5 participants