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

Dependency resolution doesn't work as expected when a project *also* uses MDX 2 #47

Open
bensmithett opened this issue Feb 14, 2022 · 5 comments

Comments

@bensmithett
Copy link

bensmithett commented Feb 14, 2022

Hello! I've read the README, I know you're looking for a maintainer and also you don't want to hear about MDX 2 problems 🙂 but wanted to FYI in case you have any obvious answers, as I believe imports.ts is giving me issues in a project where MDX 2 is present (but this plugin should be ignoring it and using MDX 1)

The issue manifests when using Storybook with storybook-builder-vite (which uses this plugin and MDX 1). This plugin attempts to require the incorrect version of mdx-js/mdx from the project root node_modules rather than the node_modules right next to it.

More details here storybookjs/builder-vite#234 and test reproduction here https://github.com/bensmithett/test-case-mdx-import

Appreciate any help or pointing me in the right direction.

@silvenon

This comment was marked as outdated.

@silvenon
Copy link
Collaborator

Oh wait, I'm wrong. Let me check more closely…

@silvenon
Copy link
Collaborator

I would say that this is a bug, yes, but I'm really scared to touch this logic because I didn't come up with it and I don't know much about dependency resolution 😔 I'll go through the logic more carefully later to see whether I can narrow it down, let me know if you'd like to help.

@bensmithett
Copy link
Author

It seems to have come from #22, specifically aleclarson@5fe7413 but I don't know enough to understand why the shift from plain imports to begin with (so I don't know what behaviour we'd be trying not to break by changing this)

Happy to help if you can shed any more light on the purpose of that imports file!

@aleclarson
Copy link
Collaborator

aleclarson commented Feb 15, 2022

but I don't know enough to understand why the shift from plain imports to begin with (so I don't know what behaviour we'd be trying not to break by changing this)

This comment should shed more light on that 👇

Both require.resolve and resolve-from use the Module._resolveLookupPaths function, which searches the node_modules of the entry module (vite/bin/vite.js) when a package is not found in the node_modules of a given root. This leads to false positives when a local clone of vite is used, since vite has preact in its devDependencies.

Even if a local clone of vite is not being used, the global node_modules are searched, which can also lead to false positives if you have preact or @mdx-js/react installed globally for some odd reason.

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

No branches or pull requests

3 participants