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

Add Bun section to Getting started #2517

Merged
merged 6 commits into from
Jul 23, 2024
Merged

Add Bun section to Getting started #2517

merged 6 commits into from
Jul 23, 2024

Conversation

karlhorky
Copy link
Contributor

@karlhorky karlhorky commented Jul 22, 2024

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Add footnote to @mdx-js/esbuild section that Bun is supported via the esbuild-compatible plugin API (I confirmed support and Bun docs confirm it too)

This actually also enables MDX eval support in scripts, which is not a straightforward thing to set up with esbuild (because the Transform API, used by programs like tsx, does not support plugins)

Alternatives considered

  1. Don't include a code block, but rather link to https://bun.sh/docs/runtime/plugins#third-party-plugins. downsides:
    1. these docs are not dedicated to @mdx-js/esbuild, so it could disappear at any time
    2. it's not a full example to be used on its own
  2. Add this instead to https://mdxjs.com/packages/esbuild/ . downsides:
    1. less discoverable (no ability to cmd-f -> search for "Bun" on "Getting Started" page)

Copy link

vercel bot commented Jul 22, 2024

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

Name Status Preview Comments Updated (UTC)
mdx ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 23, 2024 9:41am

@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (908ff45) to head (51354dc).
Report is 41 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #2517   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines         2693      2712   +19     
  Branches         2         2           
=========================================
+ Hits          2693      2712   +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wooorm
Copy link
Member

wooorm commented Jul 22, 2024

  1. Oh cool that bun already mentions it!
  2. I don’t think the esbuild plugin is currently used too much, I’d assume that bun would increase that usage; that could uncover some bugs/feature requests

@wooorm
Copy link
Member

wooorm commented Jul 22, 2024

  1. I do not understand what this has to do with eval? Scripts?

@wooorm
Copy link
Member

wooorm commented Jul 22, 2024

  1. This seems similar to how bite compares to rollup, and next compares to webpack. See https://mdxjs.com/docs/getting-started/#bundler. I think you could change that to "esbuild (or Bun)" too
  2. Also, in the same section under that heading but in the 2nd list, there's a mention of Node, you can also add a bullet after it talking about Bun. Either this point or 4. Probably, not both.
  3. This should likely be a section under https://mdxjs.com/docs/getting-started/#javascript-engines?

@wooorm
Copy link
Member

wooorm commented Jul 22, 2024

Reading your alternatives section again, have you considered my point 6? It solves both the problems you highlight with the alternatives.
The main reason I wonder is: do bun users look for esbuild plugins? Or do they look for Bun?

@karlhorky
Copy link
Contributor Author

karlhorky commented Jul 22, 2024

3. I do not understand what this has to do with eval? Scripts?

Our use case was to run a Node.js script which imports .mdx files for their exports (specifically JS exported metadata)

We wanted to continue using tsx script.ts for this, but tsx (and also the esbuild Transform API which tsx also relies upon) doesn't support esbuild plugins

So instead of taking the long way around by first using the esbuild Build API and then executing the build output (or trying to do it in memory with write: false), we tried it with Bun, which worked pretty much out of the box

Signed-off-by: Karl Horky <[email protected]>
@karlhorky
Copy link
Contributor Author

  1. This seems similar to how bite compares to rollup, and next compares to webpack. See mdxjs.com/docs/getting-started. I think you could change that to "esbuild (or Bun)" too
  2. Also, in the same section under that heading but in the 2nd list, there's a mention of Node, you can also add a bullet after it talking about Bun. Either this point or 4. Probably, not both.
  3. This should likely be a section under mdxjs.com/docs/getting-started?

Changes added for 4 and 6 in f188aa9

@wooorm
Copy link
Member

wooorm commented Jul 23, 2024

script

I was confused by this word because it is often used to differentiate between scripts vs modules. Scripts in browsers not having a way to import/export things. Or in Node, scripts using CJS.

eval

Similarly, eval is a javascript function. And MDX has an API for it (evaluate).

Our use case was to run a Node.js script which imports .mdx files for their exports (specifically JS exported metadata)

The document you are editing contains a section on Node. We have a specific package for Node (@mdx-js/node-loader). Did you consider using it?

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

I think it makes more sense to have the bun example in the bun section

docs/docs/getting-started.mdx Outdated Show resolved Hide resolved
docs/docs/getting-started.mdx Outdated Show resolved Hide resolved
docs/docs/getting-started.mdx Outdated Show resolved Hide resolved
@karlhorky
Copy link
Contributor Author

The document you are editing contains a section on Node. We have a specific package for Node (@mdx-js/node-loader). Did you consider using it?

No, actually I was not aware of this package until now. But since we were using tsx, my entry point was "how can I bootstrap tsx / esbuild to understand .mdx?"

karlhorky and others added 3 commits July 23, 2024 09:50
Co-authored-by: Titus <[email protected]>
Signed-off-by: Karl Horky <[email protected]>
Co-authored-by: Titus <[email protected]>
Signed-off-by: Karl Horky <[email protected]>
Co-authored-by: Titus <[email protected]>
Signed-off-by: Karl Horky <[email protected]>
@wooorm
Copy link
Member

wooorm commented Jul 23, 2024

But since we were using tsx, my entry point was "how can I bootstrap tsx / esbuild to understand .mdx?"

Am I correct in understanding that currently you replaced Node/TSX with Bun? You dropped them?

So, the earlier problem you had was how to use MDX with TSX but that was impossible?

Are you still interested in using MDX with TSX? Or is Bun enough?

@karlhorky
Copy link
Contributor Author

karlhorky commented Jul 23, 2024

Am I correct in understanding that currently you replaced Node/TSX with bun?

So, the earlier problem you had was how to use MDX with TSX but that was impossible?

Yes, from the reply in the tsx issue, I concluded that it is not possible to use @mdx-js/esbuild as an esbuild plugin with tsx.

For us, switching from tsx to Bun has no downsides and gives us a perf boost (we've done this for other scripts using tsx before too - for import.meta.dirname and import.meta.filename support)

Are you still interested in using MDX with TSX? Or is Bun enough?

Although we already have a solution for our projects, I'm interested in the option for other users / the ecosystem! It could potentially be an important thing for tsx users to have. Maybe with the Node.js loader it would be possible somehow?

@wooorm
Copy link
Member

wooorm commented Jul 23, 2024

for import.meta.dirname and import.meta.filename support

(Personally I don’t see the need for them, I believe URLs with import.meta.url is a very welcome improvement over paths, which have windows/unix compat differences, the possibility of paths being relative is also a downside, and URLs being specced by WHATWG is nice)

Maybe with the Node.js loader it would be possible somehow?

TSX is a Node loader and we have a Node loader too. You can use two loaders together

Comment on lines 814 to 817
preload = ["./bunMdxEsbuild.ts"]
```

```js twoslash path="bunMdxEsbuild.ts"
Copy link
Member

Choose a reason for hiding this comment

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

One thing I wonder about is this casing in the filename? Is this a very strong recommendation by bun, to use camelcasing? Seems like that would fail on some file systems?

Should the name include esbuild, or is that a side effect of what the focus is: Bun + MDX?

How about a short:

Suggested change
preload = ["./bunMdxEsbuild.ts"]
```
```js twoslash path="bunMdxEsbuild.ts"
preload = ["./mdx.ts"]
```
```js twoslash path="mdx.ts"

Copy link
Member

Choose a reason for hiding this comment

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

bun-preload.ts also seems viable?

Copy link
Contributor Author

@karlhorky karlhorky Jul 23, 2024

Choose a reason for hiding this comment

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

Is this a very strong recommendation by bun, to use camelcasing?

It was my own choice. Camel case in filenames are pretty common in JavaScript and TypeScript ecosystems. Seeing beginners work with camel case on a variety of operating systems, the errors that it causes are not very common and they can be warned against using ESLint and other tools. So I'm pretty pro-camel case I guess.

I think mdx.ts is not expressive enough. If it should be lowercase (kebab case?) then at least bun-mdx.ts or bun-mdx-plugin.ts

Copy link
Member

@wooorm wooorm Jul 23, 2024

Choose a reason for hiding this comment

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

OK, I’m very 👎 on camelcase in filenames, I only sometimes see React people use it more recently. It’s not used anywhere in the things I work on.

Did you see my 2nd suggestion, bun-preload.ts?

My acceptable preferences in order are: bun-preload.ts, mdx.ts, bun.ts, bun-mdx.ts. All fine. They all answer “wait, why is this file here again?”. Can you choose which one you prefer?

I think the inclusion of “plugin” in bun-mdx-plugin.ts is unneeded. Similar to “esbuild” bunMdxEsbuild.ts. These terms have to do with how that file currently works internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, switched to bun-mdx.ts in 3de41ee

Signed-off-by: Karl Horky <[email protected]>
@wooorm wooorm changed the title Add short Bun section to end of @mdx-js/esbuild section Add Bun section to Getting started Jul 23, 2024
@wooorm wooorm merged commit 37318de into mdx-js:main Jul 23, 2024
6 checks passed
@wooorm
Copy link
Member

wooorm commented Jul 23, 2024

Thank you!

@wooorm wooorm added 📚 area/docs This affects documentation 💪 phase/solved Post is done labels Jul 23, 2024
@karlhorky
Copy link
Contributor Author

Glad to help, thanks for the review and merge!

@karlhorky karlhorky deleted the patch-2 branch July 23, 2024 09:46
@karlhorky
Copy link
Contributor Author

cc @Jarred-Sumner Bun in MDX docs 😍

@karlhorky karlhorky mentioned this pull request Jul 23, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 area/docs This affects documentation 💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

5 participants