Skip to content
This repository has been archived by the owner on Mar 23, 2023. It is now read-only.

Plugin does not work with next-compose-plugins #18

Closed
connor-baer opened this issue Aug 10, 2019 · 9 comments
Closed

Plugin does not work with next-compose-plugins #18

connor-baer opened this issue Aug 10, 2019 · 9 comments

Comments

@connor-baer
Copy link

next-mdx-enhanced does not seem to play nice with next-compose-plugins. When the two plugins are used together, next-mdx-enhanced does not seem to have any effect:

  1. mdx pages return a 404 error
  2. When importing mdx frontmatter in other pages, Next fails to parse the file and complains about a missing loader.

Steps to reproduce

I've created a small app to reproduce the issue: https://github.com/connor-baer/next-mdx-enhanced-compose-plugins.

  1. Bootstrap a new Next app with npx create-next-app
  2. Install the two plugins yarn add next-mdx-enhanced next-compose-plugins
  3. Add a custom next.config.js file using the two plugins:
const withPlugins = require('next-compose-plugins');
const withMdxEnhanced = require('next-mdx-enhanced');

const mdxConfig = {};

module.exports = withPlugins([[withMdxEnhanced, mdxConfig]]);
  1. Add an mdx file
  2. Start the server with yarn dev and try to load the mdx file
@connor-baer
Copy link
Author

connor-baer commented Aug 10, 2019

Alright, I played around a bit more, looked at the source code of different plugins and found a way to make it work.

TL;DR: Here's an example that works:

const withPlugins = require('next-compose-plugins');
const withMdxEnhanced = require('next-mdx-enhanced');
const withBundleAnalyzer = require('@next/bundle-analyzer');

const mdxConfig = {
  remarkPlugins: [],
  rehypePlugins: []
};

const bundleAnalyzerConfig = {
  enabled: process.env.ANALYZE === 'true'
};

module.exports = withPlugins([
  [withBundleAnalyzer, bundleAnalyzerConfig]
  withMdxEnhanced(mdxConfig),
]);
  1. Call withMdxEnhanced with the config. next-mdx-enhanced's API differs from most other Next plugins in that it first accepts its own config object and then returns a function that accepts the Next config object. Most other plugins accept the Next config object directly and extract their configuration from there.
  2. Move withMdxEnhanced to the last place in the list of configs. There can be a conflict with other plugins when withMdxEnhanced is applied first. I was using next-offline, next-optimized-images, next-transpile-modules, and @next/bundle-analyzer (I didn't dig deeper into which one was causing the issue).

Personally, I'd like next-mdx-enhanced to adopt the same API as other plugins, but of course this would be a breaking change.

@jescalan
Copy link
Contributor

We have purposefully adopted a different API from what next-compose-plugins supports, and I would eventually like to start a conversation about changing the way that next-compose-plugins expects for plugin APIs to work, because I am fairly convinced that the plugin pattern I endorse is objectively better, and it's also the pattern used by zeit and all "official" plugins.

It seems like you figured out what our pattern is already, and I'm sure you can imagine why we wouldn't want to change it. Embedding your plugin-specific configuration values directly on to the primary config for nextjs itself is messy, dangerous, easily prone to breaking your plugin and/or entire app any time next changes their config due to potential conflicts. When your plugin's config is used via its own separate options, you get clean, clear separation between config for your plugin and config for nextjs, and zero chance of any conflicts or breakage.

@peduarte
Copy link

There's a Next.js RFC for better plugins vercel/next.js#9133

Thought I'd drop it here as maybe it'll impact this issue.

@benwinding
Copy link

We have purposefully adopted a different API from what next-compose-plugins supports

This is important and should at least be added to the README then...

@jescalan
Copy link
Contributor

@benwinding - If you start down this road, it is a very slippery slope. What if someone else makes another compose plugins library that breaks in a different way? Do I need to document that too? What if only 10 people use it - do I need to come up with a cutoff for "if this number of people use this other library, we document it in our readme"? It's not really a policy that I want to be enforcing or figuring out.

If a user has decided to adopt a non-standard plugin loading library that works in an explicitly different way than next.js endorses, it is on that user to ensure that the plugins they want to use work with it, not on every plugin in the ecosystem to adopt the plugin loading library's patterns, or document whether it works with them or not in their readmes.

@benwinding
Copy link

If you start down this road, it is a very slippery slope. What if someone else makes another compose plugins library that breaks in a different way? Do I need to document that too? What if only 10 people use it - do I need to come up with a cutoff for "if this number of people use this other library, we document it in our readme"? It's not really a policy that I want to be enforcing or figuring out.

@jescalan I see your point and agree that it is a slippery slope on how detailed to document and you shouldn't have to support everything.

However, this is a highly common use-case (as shown by the activity on this issue). It also took me longer than I'd like to admit to figure it out as it was difficult to debug what was wrong. I think there's 2 options that would help future users:

  • Add a small example or explanation in Usage & Options in the Readme.
  • Add better error logging when the plugin is loaded, so if it's badly configured, you can tell.

You shouldn't have to support and document every plugin system and use case, nor come up with policies on what is supported, but a small line of text in the readme can save everyone else a lot of time.

Anyway, thanks for developing this, appreciate your efforts and quick responses.

@jescalan
Copy link
Contributor

However, this is a highly common use-case (as shown by the activity on this issue)

There's one thumbs up on the issue, and it was from you. We haven't seen a comment on this issue before the one you made just now in nearly two years. Unfortunately, this is not a highly common use case, in fact the activity on this issue tells me that it's a very rare use case.

As I mentioned in the last comment, I'm not open to adding information about this to the readme. If you can come up with a way to have the plugin error if its not configured correctly, I'd be happy to hear a proposal for this or review a PR.

I understand that it can be frustrating when something doesn't work the way you expected and I have myself experienced this many, many times throughout my career. Sometimes, certain tools are not clearly documented, or are built with poor developer experience, and they can be improved as a result of user confusion. But I do not think in this case that the onus is on next-mdx-enhanced to make changes. The readme is quite clear about how the plugin is used and configured. Searching issues for next-compose-plugins would turn this thread up quickly. And checking the readme for next-compose-plugins, it first of all has not had a genuine update in 2+ years, so appears to be unmaintained, but also it does not really make any mention of the fact that any given plugin could not work with it, despite the fact that many plugins, not only this one, are configured in the same way, and none of them would work with it the way they propose in their docs.

If you do want to make a change to documentation to make this less confusing for others in the future, I would recommend proposing a change to the readme for next-compose-plugins and adjusting their readme to account for this plugin configuration pattern, which, as I mentioned before, is not unique to this plugin and is in fact utilized by several vercel-native plugins as well.

While we're here, I would also recommend dropping next-compose-plugins -- as mentioned earlier, it's clearly not actively maintained, and this is not the last time you will run into this problem with the way it works. If you're looking for an alternative, I can offer the following code that works with all plugin styles and is much lighter and more clear than next-compose-plugins:

// next.config.js
module.exports = () => {
  const plugins = [
    pluginOne(),
    pluginTwo,
    pluginThree()
  ]
  return plugins.reduce((acc, next) => next(acc), {
    /* normal nextjs config options here */
  })
}

And finally, I would also recommend dropping this plugin, since it is also unmaintained and will eventually be incompatible with the latest version of nextjs, and give next-mdx-remote a shot, as the note in the readme indicates 😀

@benwinding
Copy link

There's one thumbs up on the issue, and it was from you. We haven't seen a comment on this issue before the one you made just now in nearly two years.

Well, I was referring to the solution posted #18 (comment) , which has 6 likes, but anyway it doesn't matter.

As I mentioned in the last comment, I'm not open to adding information about this to the readme.

I understand your point of view, as everything you add is another thing to maintain, and it's not this project's responsibility. I guess I would prefer to make the consumer's life a tiny bit easier, but everything's a trade-off and you've clearly thought this through and have had experience with this before.

// next.config.js
module.exports = () => {
  const plugins = [
    pluginOne(),
    pluginTwo,
    pluginThree()
  ]
  return plugins.reduce((acc, next) => next(acc), {
    /* normal nextjs config options here */
  })
}

That's an interesting alternative, thanks for sharing!

And finally, I would also recommend dropping this plugin, since it is also unmaintained and will eventually be incompatible with the latest version of nextjs, and give next-mdx-remote a shot, as the note in the readme indicates.

Yes, I have looked at this package, but it seems a bit more complex, and handles many use cases. Is it a drop in replacement? I just want a few mdx files in the /pages directory that can have a layout applied to them... I couldn't find any of the many examples in next-mdx-remote that show that...

Appreciate the discussion anyway 👍

@jescalan
Copy link
Contributor

mdx-remote is not a drop-in enhancement, its a significant migration. there is an official nextjs example implementation though, if it helps!

https://github.com/vercel/next.js/tree/canary/examples/with-mdx-remote

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

No branches or pull requests

4 participants