brillout / vite-plugin-mdx

Vite Plugin for MDX
MIT License
113 stars 36 forks source link

feat: resolve dependencies relative to config.root #10

Closed aleclarson closed 3 years ago

aleclarson commented 3 years ago

When using a local clone of this plugin, it can fail to resolve react and @mdx-js/react, so this PR uses config.root (from Vite config) when resolving those packages.

brillout commented 3 years ago

Good idea. One thing though: how about using require.resolve with the paths options https://nodejs.org/api/modules.html#modules_require_resolve_request_options instead of adding a dependency?

aleclarson commented 3 years ago

Not worth the effort. The dependency is tiny, popular, and battle-tested: https://github.com/sindresorhus/resolve-from/blob/main/index.js

brillout commented 3 years ago

As a reader it's an order of magnitude easier and quicker to read:

function resolveFrom(source, from) {
  try {
    return require.resolve(source, {paths: [from]})
  } catch(err) {
    return null
  }
}

Than having to look up what resolve-from is about. (E.g. I had to do this while reviewing your PR.)

I find code riddled with tiny dependencies non-significantly harder to read.

Objections?

aleclarson commented 3 years ago

Than having to look up what resolve-from is about.

Not a big deal to me, but if you prefer it, you can push a follow-up commit after this is merged. No PR necessary. 👍

brillout commented 3 years ago

follow-up commit

Will do.

I find that these small things add up over time.

brillout commented 3 years ago

Will do.

Done. Feel free to release a new version.