Skip to content

docs(openapi-ts): add docs for using Node.js API in web projects #1195

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

Conversation

psychedelicious
Copy link
Contributor

Changes

Update docs with examples for using the Node.js API in a project with a browser build target.

How to Review

I'm not sure if this deserved to be in the docs as it's only tangential to openapi-typescript, and I imagine most devs know how to set this up already.

I, however, didn't know, and it took me an hour of going down stack-overflow garden paths before discovering ts-node --esm, so I figured I'd propose the changes.

Checklist

  • [ ] Unit tests updated
  • README updated
  • [ ] examples/ directory updated (only applicable for openapi-typescript)

@changeset-bot
Copy link

changeset-bot bot commented Jul 2, 2023

🦋 Changeset detected

Latest commit: e20f084

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openapi-typescript-docs Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@drwpow drwpow merged commit c732542 into openapi-ts:main Jul 3, 2023
@github-actions github-actions bot mentioned this pull request Jul 3, 2023
@drwpow
Copy link
Contributor

drwpow commented Jul 10, 2023

I’ll admit I actually didn’t read these too carefully before approving—I assumed they were related to #1193 which I’m fine with some redundancy. However, this PR actually does not add helpful info to the Node.js API docs; rather, it adds some confusing information.

To be more specific:

  • The term “browser build target” doesn’t mean something universally; this looks more like confusion over CJS required for a webpack config vs running Node in ESM (which is already “browser build target”-ready).
  • ts-node is NOT required to run this script. Only modern Node in ESM mode. In fact, I generally would not recommend people run ts-node for… anything, really. If people need a TS runtime, vite-node is probably better overall. But again, since this isn’t needed, I’d rather recommend nothing
  • { "type": "module" } is actually a good callout! I think this is an important oversight you caught that needed to be in the docs. The other code is just hiding this key element.

So to summarize, I think really we just need to add to the docs ESM mode is required and that’s the main takeaway here. I don’t think any other element of this PR is unique or new, but please correct me if I’m mistaken.

Proposal

  • This entire section should just be removed—it just repeats the same setup from the top of the page
  • The critical { "type": "module" } (ESM mode) should be specified at the top of the page in necessary setup. Probably as a warning callout (I think I’ve just been using blockquotes with the ⚠️ emoji).
  • There should be a separate section for CJS projects just saying it’s not supported. Or if it is, I’d probably recommend vite-node as a last resort.

I’d be happy to accept a PR with those changes, if you’d like!

Also relevant to #1217

@psychedelicious
Copy link
Contributor Author

psychedelicious commented Jul 18, 2023

@drwpow
Thanks for that explanation. I'm not sure why, but in the app in which I was using openapi-typescript, I had to do all the things I added to the docs via this PR (node 18, maybe that is the issue). I suppose there is some unique configuration in our app.

I do intend to fix the docs per your advice, but am in the middle of a big launch. I won't have time until next weekend.

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.

2 participants