Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Hook Reference: Automate a reference resource #6454

Merged
merged 17 commits into from
Mar 17, 2021
Merged

Hook Reference: Automate a reference resource #6454

merged 17 commits into from
Mar 17, 2021

Conversation

psealock
Copy link
Collaborator

@psealock psealock commented Feb 26, 2021

Fixes #2924

Process all document blocks and compile those that describe hooks into a JSON file for easy data source. This data source can be used to populate a page in the WooCommerce Developer Portal so that extension developers can easily find documentation about available hooks. cc @loranallensmith

Next Steps

  1. Add @hook document blocks to remaining hooks in JavaScript.
  2. Figure out how to handle PHP hooks introduced in wc-admin. Should they simply be included in Core's references? Or should they be a part of this process?
  3. Write a markdown file describing requirements for developers adding or changing hooks.
  4. Create Developer Portal page.

Detailed test instructions:

  1. Run the script
npm run create-hook-reference  
  1. Check out the resulting JSON in bin/hook-reference/data.json
[
    {
        "description": "List of homepage stats enabled by default",
        "sourceFile": "https://github.com/woocommerce/woocommerce-admin/blob/1682331e6fe5e898d41049baf41a7da4270a6865/client/homescreen/stats-overview/defaults.js#L5-L16",
        "name": "woocommerce_admin_homepage_default_stats",
        "example": "addFilter( 'woocommerce_admin_homepage_default_stats', 'plugin-domain', ( defaultStats ) => defaultStats.filter( ( stat ) => stat !== 'jetpack/stats/views' ) );"
    }
]
  1. Make sure the source file url works.
  2. Should any other information be added or improved?

@ilyasfoo
Copy link
Contributor

ilyasfoo commented Mar 2, 2021

@psealock I've tested running npm run create-hook-reference with node v12.21.0, and it gives the following error:

Error: Cannot find module 'fs/promises'
Require stack:
- .../plugins/woocommerce-admin/bin/hook-reference/index.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:815:15)
    at Function.Module._load (internal/modules/cjs/loader.js:667:27)
    at Module.require (internal/modules/cjs/loader.js:887:19)
    at require (internal/modules/cjs/helpers.js:74:18)
    at Object.<anonymous> (.../plugins/woocommerce-admin/bin/hook-reference/index.js:2:38)

I suspected this was because fs/promises alias is not in v12. I've tested to change require( 'fs/promises' ); to require( 'fs' ).promises and it works well with both v12 and v14.

Not a hard requirement, but would it be better to use the one that's supported by node v12 and above?

@loranallensmith
Copy link
Contributor

This is awesome @psealock! 🎉 Thank you for putting it together. I especially love the way the sourceFile URL links to the canonical URL and highlights the lines in question. 🤩

Screen Shot 2021-03-03 at 3 27 50 PM

const { resolve } = require( 'path' );
const createData = require( './data' );

async function getFilePaths( dir ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Promise.all, map, reduce, and recursion all in a ~10 line async function. 👏🏻

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤣 There was significant help from stack overflow

Copy link
Contributor

@joelclimbsthings joelclimbsthings left a comment

Choose a reason for hiding this comment

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

❤️ this @psealock . Tested and works well.

Do we have any expectations on how the generated json will be utilized? Would it useful to integrate this script into any part of the ci/build/release process?

Totally optional, but I like it when a script gives some kind of output to assure me that in executed correctly. Something like Hook reference generated at ${path/file}

@joelclimbsthings joelclimbsthings added needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. and removed status: needs review labels Mar 9, 2021
@psealock
Copy link
Collaborator Author

psealock commented Mar 15, 2021

Not a hard requirement, but would it be better to use the one that's supported by node v12 and above?

Good call @ilyasfoo and thanks for the tip. I fixed that in 66b205c

@psealock
Copy link
Collaborator Author

Thanks for the review @joelclimbsthings

Totally optional, but I like it when a script gives some kind of output to assure me that in executed correctly. Something like Hook reference generated at ${path/file}

Great idea! I added some fancy logs in 3f2354b.

Screen Shot 2021-03-15 at 1 43 35 PM

Do we have any expectations on how the generated json will be utilized?

Since you can easily make a GET request to .json pages on Github, I think it would be used as an external resource loaded by what ever page we create to make these hooks viewable and searchable.

Would it useful to integrate this script into any part of the ci/build/release process?

Eventually, I think so.

@psealock psealock added status: needs review and removed needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. labels Mar 15, 2021
@joelclimbsthings
Copy link
Contributor

💯 Those are 🌶️ . 🚢

@psealock psealock merged commit b17aab1 into main Mar 17, 2021
@psealock psealock deleted the add/doc-parsing branch March 17, 2021 00:27
ObliviousHarmony pushed a commit to woocommerce/woocommerce that referenced this pull request Mar 18, 2022
…e-admin#6454)

* read sample doc

* npm cli

* work with arrays

* Its happening

* better naming

* cleanup

* moar cleanup

* new line

* better

* save

* fixup rebase error

* package lock update

* node 12 usage

* add changelog

* fancy logs

* update package lock

* changelog in right place
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automated hook reference
4 participants