dinero.js icon indicating copy to clipboard operation
dinero.js copied to clipboard

docs: adding api-extractor

Open johnhooks opened this issue 3 years ago • 7 comments

adding @microsoft/api-extractor will allow for rolling up declaration files, tracking api changes, and generating api documentation.

johnhooks avatar Nov 17 '21 14:11 johnhooks

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/dinerojs/dinerojs/2fuXbAX3EQ88xA377mYVNYf4P2kP
✅ Preview: https://dinerojs-git-fork-johnhooks-add-api-extractor-dinerojs.vercel.app

vercel[bot] avatar Nov 17 '21 14:11 vercel[bot]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 814c449a1b5fd0c0597024a0c8b1247e2c2ef6ec:

Sandbox Source
@dinero.js/example-cart-react Configuration
@dinero.js/example-cart-vue Configuration
@dinero.js/example-pricing-react Configuration
@dinero.js/example-starter Configuration

codesandbox-ci[bot] avatar Nov 17 '21 14:11 codesandbox-ci[bot]

What's this part for? If we can have a JSON file instead (I think it's possible) which we can then leverage in the site with MDX, it's better.

@sarahdayan, it does generate a JSON file but it is pretty complex, it can be used by the document generator to create a set of linked markdown files. Though it could be read with the api-extractor-model package and used how ever you want. Without any extra config the markdown output looks like this example.

By the way, all the builds failed because I didn't update the lock file. I'll do that in future commits, but I assume that file will be tricky and probably need to be generated after an upstream merge, right before this PR is accepted.

johnhooks avatar Nov 17 '21 14:11 johnhooks

I have added api-extractor to core, the most interesting for review is the packages/core/etc/core.api.md file. Its for tracking api changes, alert for errors, and show what is public but isn't documented.

I haven't added a script to run api-extractor yet, but if you want to play with it, you can run it with the following. Right now it has a bunch of warning, but most are about needing to add the @public flag to all the comments of the public facing api.

  1. cd packages/core
  2. npx tsc -p .
  3. npx api-extractor run --local --verbose

This also generates a rolled up declaration file under packages/core/dist/code.d.ts.

johnhooks avatar Nov 17 '21 21:11 johnhooks

Would all of core be considered @internal?

johnhooks avatar Nov 17 '21 22:11 johnhooks

Thanks a lot for PRing @johnhooks. I'm unable to look into it for the moment but I'll have more time next week.

sarahdayan avatar Dec 13 '21 13:12 sarahdayan

Totally understand. I started trying to use it in some private projects and it's been educational. I'll try to push a new commit that implements some of what I've learned. Also I will start with getting it working in the dinero.js package. It's the most public facing package and make the most sense for implementation.

johnhooks avatar Dec 13 '21 14:12 johnhooks

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
dinerojs ✅ Ready (Inspect) Visit Preview Nov 6, 2022 at 4:18PM (UTC)

vercel[bot] avatar Oct 19 '22 16:10 vercel[bot]

@sarahdayan I modified packages/dinero.js to use api-extractor. Now that package's scriptbuild:types generates a single type file into dinero.js/packages/dinero.js/dist/dinero.js.d.ts. I did need to make a few changes to incorporate api-extractor, some config files, source comments, and some modifications to the tsconfig.json in the package folder. Api-extractor uses the location of the tsconfig.json to discover a projects location. I am sure I could override the defaults to use the tsconfig.declaration.json, if that is important. Also, I totally understand this is a super low priority. I restarted my investigation of this as a way to learn how to merge a fork with an upstream repo.

johnhooks avatar Oct 21 '22 02:10 johnhooks

I added api-extractor to @dinero.js/currencies. Are all the currencies generated by a script or written by hand? To quiet api-extractor warnings about commenting which exports are public I need to add a @public comment to all the currencies.

johnhooks avatar Oct 23 '22 12:10 johnhooks

~~@sarahdayan over all, my feelings about this solution is that it's complex and api-extractor is built to be incorporated into monorepos managed by rush. I'm going to look at https://github.com/wessberg/rollup-plugin-ts, and see if maybe it's a better solution to rollup the declarations, without all the overhead and config of this PR.~~

EDIT: Maybe I'm being to critical. It would be very handy to author most of the documentation in the source comments, and be able to pull that and the type declaration into a mdx file and then add additional resources and examples in that file.

johnhooks avatar Nov 05 '22 16:11 johnhooks

@sarahdayan when I rebased into main, it changed my main branch, not the add-api-extractor branch. Sorry I'm very new to the PR/git process.

johnhooks avatar Nov 06 '22 14:11 johnhooks

@johnhooks No worries, Git is hard.

Here's the process:

  1. Remove your local main branch (git branch -d main)
  2. Checkout main (git checkout main)
  3. Pull the latest changes (git pull -r)
  4. Checkout your branch (git checkout add-api-extractor)
  5. Rebase onto main (git rebase main)
  6. Force push your branch (git push add-api-extractor --force)

sarahdayan avatar Nov 06 '22 15:11 sarahdayan

@sarahdayan is that supposed to take me though a long interactive process for each commit? Just making sure I'm doing this right.

johnhooks avatar Nov 06 '22 15:11 johnhooks

@johnhooks If there are conflicts, yes. You can also merge main, it should be easier than rebasing.

sarahdayan avatar Nov 06 '22 15:11 sarahdayan

@sarahdayan I already merge upstream in https://github.com/dinerojs/dinero.js/pull/499/commits/0b541b02b0fa314b69d8203e3f9b010357f6f8e6

johnhooks avatar Nov 06 '22 15:11 johnhooks

@johnhooks That was before I made the change, so you need to do it again.

sarahdayan avatar Nov 06 '22 16:11 sarahdayan

@sarahdayan okay, I'll keep slogging through this. One issue I'm having, vscode isn't allows suggesting to open the merge editor. whats the right cli way to resolve merge conflicts

johnhooks avatar Nov 06 '22 16:11 johnhooks

@johnhooks I just use Vi, but I think you can set things up to use another editor.

If you want for this time I can handle the merge?

sarahdayan avatar Nov 06 '22 16:11 sarahdayan

@sarahdayan I think I did it!

johnhooks avatar Nov 06 '22 16:11 johnhooks

@sarahdayan awesome! This is my first big commit to an open source repo, I'm pumped!

johnhooks avatar Nov 06 '22 16:11 johnhooks

@sarahdayan I think some where in the rebase to make things work I delete the yarn.lock file! I though it was going to keep the original and just get rid of mine, but I don't see it in the repo. Sorry

johnhooks avatar Nov 06 '22 16:11 johnhooks

@johnhooks Ah, indeed, I missed that during my last review. I'll re-generate it.

sarahdayan avatar Nov 06 '22 16:11 sarahdayan

I was just working to try to do that, just checking so I make sure I know how, I would checkout the lock file from the commit right before mine and then run yarn install, add the file and commit?

johnhooks avatar Nov 06 '22 16:11 johnhooks

@johnhooks You'd need to go back to the previous commit, copy the file, get to the head, paste the file, then run yarn install.

I re-created the file so no worries.

sarahdayan avatar Nov 06 '22 16:11 sarahdayan