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

Support turbo #468

Closed
wants to merge 5 commits into from
Closed

Support turbo #468

wants to merge 5 commits into from

Conversation

arlyon
Copy link

@arlyon arlyon commented Jun 14, 2024

Hey, as promised, a draft PR! Comments welcome :)

Notes

  • I just converted the map config plugin to a plain ol' function
  • added support for ['plugin-name', { ...options }] syntax
  • split the plugin resolution into two types, the 'input' phase contains the unresolved imports
  • added a method to resolve these imports
  • allowed overriding the providerImportSource. I believe this will be mandatory when using turbo because we can't rely on webpack aliases
  • I have not ported / modified the search plugin

Questions:

  • how do we communicate to the user that if using turbo you need to have serializable options? the types here will be funky. maybe 'createMDXTurbo' that just wraps the normal fn and adds the extra constraint?
  • right now this syntax only supports default imports. I propose we ship it and come back to it if anyone needs that feature.

Still need to write docs but will come back to that once these Qs are resolved.

Testing

Here is a page on my deployed site https://github.com/arlyon/litehouse/blob/main/site/content/docs/plugins/index.mdx?plain=1#L21-L22 that uses both a remark plugin and a rehype language.

You can validate the webpack side still works here https://litehouse.arlyon.dev/docs/plugins and if you'd like to clone and build the repo you should be able to do that fairly trivially but here is also a screenshot.

image

Copy link

changeset-bot bot commented Jun 14, 2024

⚠️ No Changeset found

Latest commit: d8fc004

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

Copy link

vercel bot commented Jun 14, 2024

@arlyon is attempting to deploy a commit to the Fuma Team on Vercel.

A member of the Team first needs to authorize it.

@arlyon arlyon marked this pull request as draft June 14, 2024 11:43
@fuma-nama
Copy link
Owner

how do we communicate to the user that if using turbo you need to have serializable options? the types here will be funky. maybe 'createMDXTurbo' that just wraps the normal fn and adds the extra constraint?

This is fine, I think mentioning it on the docs is enough (since types can be hard to deal with).
But apart from passing remark and rehype plugins. For Shiki (the syntax highlighter), people might want to add their own transformers which also needs to be a function, the eco-system isn't pretty friendly for this

@fuma-nama
Copy link
Owner

right now this syntax only supports default imports. I propose we ship it and come back to it if anyone needs that feature.

Agree, maybe I can make the support for named imports like remark-math/something{name} later.

@arlyon
Copy link
Author

arlyon commented Jun 14, 2024

TY for the quick feedback. I will have a think about shiki over the weekend. I think the answer will be that user-defined transforms as inline functions are a no-go. You'd need to put them in a different package in your repo and import them.

@anthonyshew
Copy link
Contributor

Could we throw a warning in that case? Or mark the support as experimental? My thinking is that we could get support out for the folks not using transformers (I'd have to assume the majority of folks) and look forward to the wider support in the future.

@fuma-nama
Copy link
Owner

Before Turbopack goes stable, I'll mark it as experimental. Maybe we can check the passed options in development mode, then throw the warnings if unserializable data detected.

@fuma-nama
Copy link
Owner

Just an idea: How about making a configuration file for the loaders, e.g. mdx.config.ts and bundle that file on-start?
After bundling the configuration file, we can call eval it (theoretically) on the runtime of Turbopack and obtain the options for MDX loaders. I'm not sure if it against the design of Turbopack, but I think having an easier migration experience would be more motivating for people to switch to Turbopack.

@fuma-nama
Copy link
Owner

I cleaned some code and added the isSerializable function for checking the MDX options

@kijv
Copy link
Contributor

kijv commented Jun 21, 2024

In Next.js, when you able the --turbo it also sets process.env.TURBOPACK = '1'. So instead of the default value for experimentalTurbo being false should it be Boolean(process.env.TURBOPACK)?

@arlyon
Copy link
Author

arlyon commented Aug 6, 2024

Hey @fuma-nama, do you have strong opinions regarding the syntax? Is the default export syntax I proposed ok in the mean time? Regarding #468 (comment) the intention is to have closures work ootb so we should be able to remove this 'experimental' syntax eventually.

If you are on board I will rebase it and we can get it in :)

@fuma-nama
Copy link
Owner

My only concern is that the current setup still lacks flexibility, since most remark/rehype plugins are not designed to work on Turbo, people might want to pass unserialisable options.

I'm taking reference from other similar things like Content Collections (or Contentlayer). If the loader can also bundle the configuration file with Turbopack, we might be able to "evaluate" the config file in runtime, and get unserialisable options working with our MDX loader.

From what I understand, we cannot pass unserialisable options to loaders, because there's a Rust layer between next config and loaders. But if it's a JavaScript loader evaluating another JavaScript config, it should be possible.

Sorry if it's too much change, highly appreciate your efforts to improve the build system ;)

@fuma-nama fuma-nama force-pushed the dev branch 2 times, most recently from 16c01e4 to 17746a6 Compare August 18, 2024 15:34
@fuma-nama fuma-nama force-pushed the dev branch 2 times, most recently from 8b4c251 to 3d1ec96 Compare August 27, 2024 12:31
@fuma-nama
Copy link
Owner

Now I'm working actively on Fumadocs MDX, a few news:

  • the new MDX v10 can support Turbopack
  • Content Collections also works with Turbopack

I'm trying to use esbuild to bundle the configuration file (source.config.ts), and load it from MDX loaders. This makes it possible to load unserializable options.

If Turbopack has its own functionality to load TypeScript file/config (e.g. a bundle() function), I'll willing to switch from esbuild to Rust-based solutions.

@fuma-nama
Copy link
Owner

Closing in favour of #778

@fuma-nama fuma-nama closed this Aug 31, 2024
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.

4 participants