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

Improved documentation & types #1849

Closed
wants to merge 13 commits into from
Closed

Improved documentation & types #1849

wants to merge 13 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 2, 2023

  1. Improved documentation for lib/node.d.ts, lib/comment.d.ts and lib/declaration.d.ts
  2. Changed return type for Node.clone() from this to Node, reason: this is the reference to the original node, and having type of original node instance on a duplicated node instance is not correct method. click here to view

lib/node.d.ts Outdated Show resolved Hide resolved
lib/node.d.ts Outdated Show resolved Hide resolved
lib/comment.d.ts Outdated Show resolved Hide resolved
lib/node.d.ts Outdated Show resolved Hide resolved
lib/declaration.d.ts Outdated Show resolved Hide resolved
lib/comment.d.ts Outdated Show resolved Hide resolved
lib/declaration.d.ts Outdated Show resolved Hide resolved
lib/declaration.d.ts Outdated Show resolved Hide resolved
lib/comment.d.ts Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Jul 2, 2023

I have pushed the changes, they appear on my fork but not appearing here... @ai

@@ -30,18 +30,21 @@ declare namespace Comment {
export { Comment_ as default }
}


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

* ```
*/
value: string

/**
* `true` if the declaration has an `!important` annotation.
* The `important` property represents a boolean value. If true,
Copy link
Member

Choose a reason for hiding this comment

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

Need to remove name duplication here as well

@ai
Copy link
Member

ai commented Jul 2, 2023

I have pushed the changes, they appear on my fork but not appearing here... @ai

Strange.

What we can try to fix the problem:

  1. Close this PR and open another from your branch (but GitHub will not suggest you a branch in yellow banner, you will need to click New PR and manually select branch). If not, I will get changes manually from your branch, don't worry (just don't delete branch in your repo).
  2. Avoid do merge main commits. They are not very stable.
  3. After your PR was accepted, I prefer to delete my fork (copy of PostCSS repo in your account) and create a fork again for the next PR.

@ghost ghost closed this Jul 2, 2023
@ghost ghost mentioned this pull request Jul 2, 2023
@idoros
Copy link
Contributor

idoros commented Jul 12, 2023

Changed return type for Node.clone() from this to Node, reason: this is the reference to the original node, and having type of original node instance on a duplicated node instance is not correct method

@vikaskaliramna07 - can you expand on the reasoning behind this? When an instance of postcss.Root is cloned, the returned copy is also a Root node. This new return type require casting back into the original type. How can clone change the type of the returned instance?

Also you changed the jsdoc on the clone function to:

  • The clone method creates clone of an existing node,
  • which includes all the properties and their values, that
  • includes raws but not type.

From what I can see in the code, there is no specific handling of type field, the clone simply creates a new instance from the prototype and then copy self owned properties from the original to the clone. This clone method is inherited to other types of nodes and causes a regression in code that uses it.

This pull request was closed.
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.

None yet

2 participants