-
-
Notifications
You must be signed in to change notification settings - Fork 597
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
Add jsdoc comments for migrate #255
Conversation
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
Nice, thanks! Can you run the jsdocs command and commit? |
Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@dylanonelson thanks for the work! As this PR is quite huge (due to generation of docs), I'd like to invest at least some time to go through the comments :) And thanks to your PR, I think for the future doc generation should be part of CI not of each PR. |
Yup, +1 |
OK sounds good. |
lib/migrate/resolve/resolve.js
Outdated
* based on what is already in `resolve.root` | ||
* | ||
* @param {NodePath} p — wrapped ast property node that represents the `resolve` property | ||
* @return {NodePath} p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay had an extensive review, most of the stuff are okay, except some small typos regarding to the syntax we have. Collection
is identical to Node
, but we use Node
- ast - returns JScodeshift ast for that. Thank you so much for the PR, its huge, extensive, important and well done!
* Transform for BannerPlugin arguments. Consolidates first and second argument (if | ||
* both are present) into single options object. | ||
* | ||
* @param {object} j — jscodeshift top-level import |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object
* | ||
* @param {object} j — jscodeshift top-level import | ||
* @param {Collection} ast — jscodeshift Collection to transform | ||
* @returns {Collection} ast — jscodeshift Collection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node
* @param {object} j — jscodeshift top-level import | ||
* @param {Node} node — ast node to check | ||
* @param {string} pluginName | ||
* @returns {boolean} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boolean
, Object
, String
, could you add pluginName - name of the plugin
?
* | ||
* Transform for ExtractTextPlugin arguments. Consolidates arguments into single options object. | ||
* | ||
* @param {object} j — jscodeshift top-level import |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object
, Node
for ast
lib/migrate/index.js
Outdated
@@ -48,7 +48,7 @@ function transformSingleAST(ast, source, transformFunction) { | |||
* @param { String } source - Source file contents | |||
* @param { Array<Function> } transformations - List of trnasformation functions in defined the | |||
* order to apply. By default all defined transfomations. | |||
* @param { Object } options - Reacst formatting options | |||
* @param { Object } options - Recast formatting options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add this JSdoc too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, no problem. Do you mean the jsdoc for the properties of the options
object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you forgot to do this/add the extra *. And Array<Function>
is wrong, should be Function[]
* Transform which removes the json loader from all possible declarations | ||
* | ||
* @param {object} j — jscodeshift top-level import | ||
* @param {Collection} ast — jscodeshift Collection to transform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object and node
* Remove the loader with name `name` from the given NodePath | ||
* | ||
* @param {NodePath} path — NodePath to remove the loader from | ||
* @param {string} name — the name of the loader to remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String and Node
lib/migrate/resolve/resolve.js
Outdated
* | ||
* Transform which consolidates the `resolve.root` configuration option into the `resolve.modules` array | ||
* | ||
* @param {object} j — jscodeshift top-level import |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big O in Object and Node
lib/migrate/resolve/resolve.js
Outdated
* Add a `modules` property to the `resolve` object or update the existing one | ||
* based on what is already in `resolve.root` | ||
* | ||
* @param {NodePath} p — wrapped ast property node that represents the `resolve` property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node and elaboration in returns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean here by “elaboration in returns”?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elaborate what p returns a bit more 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a description to this @returns
tag. Do you want me to add descriptions for all of them? In cases where it seemed self-explanatory I omitted the description.
* | ||
* Transform which adds a `sourceMap: true` option to instantiations of the UglifyJsPlugin | ||
* | ||
* @param {object} j — jscodeshift top-level import |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big O and Node
Planned to have this merged and fixing that myself, but let's just fix it now 🍶 |
OK I did my best to make all the requested changes and also scanned through all the docs to make sure they matched your description— Thanks for the review! |
Re commented, thanks for checking it out! |
Thanks! Will re-review later today! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, sorry for getting back so late! So there's some small nits left, sorry for being so strict!
@@ -219,7 +248,7 @@ module.exports = function(j, ast) { | |||
.forEach(createArrayExpressionFromArray); | |||
|
|||
/** | |||
* Finds loaders with options encoded as query string and replaces it with options obejct | |||
* Find loaders with options encoded as a query string and replace the string with an options object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interactive verb? replaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused about this. You want to change replace
to replaces
but leave Find
in the same sentence? It seems like they should match. I skimmed through some of the other comments and it looks like sometimes the doclets use the replace
version and sometimes they use the replaces
version. If you want, I can make them all consistent. Or there may be something special about this one case that I don't understand—just let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its a small pick, so no worries 👍
@@ -249,7 +277,7 @@ module.exports = function(j, ast) { | |||
.replaceWith(createLoaderWithQuery); | |||
|
|||
/** | |||
* Finds nodes with `query` key and replaces it with `options` | |||
* Find nodes with a `query` key and replace it with `options` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interactive indication, so: replaces
* @param {Object} j - jscodeshift top-level import | ||
* @param {Node} node - ast node to check | ||
* @param {String} pluginName - name of the plugin | ||
* @returns {Boolean} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you give a bit of description what the boolean indicates in the @returns
?
lib/migrate/loaders/loaders.js
Outdated
* @return {Node} [] | ||
* | ||
* @param {Node} path - object expression ast | ||
* @returns {Node} path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what the returns value does/is?
lib/migrate/loaders/loaders.js
Outdated
* @param {Node} path [description] | ||
* @return {Node} [description] | ||
* @param {Node} path - identifier ast | ||
* @returns {Boolean} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate? Thanks 🎧
lib/migrate/loaders/loaders.js
Outdated
* @param {Node} p [] | ||
* @returns {*} [] | ||
* @param {Node} p - object expression ast that has a key for either 'preLoaders' or 'postLoaders' | ||
* @returns {Node} p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be a 10/10 if there was a small explaining part here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for bearing with me on this! 💙
@@ -219,7 +248,7 @@ module.exports = function(j, ast) { | |||
.forEach(createArrayExpressionFromArray); | |||
|
|||
/** | |||
* Finds loaders with options encoded as query string and replaces it with options obejct | |||
* Find loaders with options encoded as a query string and replace the string with an options object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its a small pick, so no worries 👍
@ematipico and @fokusferit can you re-review? Thanks bois! |
@dylanonelson Thanks for your update. I labeled the Pull Request so reviewers will review it again. @ematipico Please review the new changes. |
What kind of change does this PR introduce?
JSdocs for migrate
Did you add tests for your changes?
No
If relevant, did you update the documentation?
n/a
Summary
Resolves issue #219
Does this PR introduce a breaking change?
No
Other information
Did my best to describe everything correctly, but I may have made mistakes. I noticed the existing jsdoc comments were a bit inconsistent, so I went with what made sense to me.