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 configure option to inferNamedImports #37

Open
0xDing opened this issue Oct 18, 2021 · 5 comments
Open

add configure option to inferNamedImports #37

0xDing opened this issue Oct 18, 2021 · 5 comments

Comments

@0xDing
Copy link

0xDing commented Oct 18, 2021

Clear and concise description of the problem

For monorepo projects, it is likely that both react and preact packages required in the dependencies, so passing resolveImport('preact', root) may be problematic

Suggested solution

add configure option

Alternative

yarn patch

diff --git a/dist/imports.js b/dist/imports.js
index 8f16352ab74f329dd01389d416059754bbf0a99f..418fc1b9b83b7111799c2fe4acd7564ba351df75 100644
--- a/dist/imports.js
+++ b/dist/imports.js
@@ -42,9 +42,7 @@ function assertImportExists(name, cwd) {
 }
 exports.assertImportExists = assertImportExists;
 function inferNamedImports(root) {
-    return resolveImport('preact', root)
-        ? { preact: ['h'], '@mdx-js/preact': ['mdx'] }
-        : { react: 'React', '@mdx-js/react': ['mdx'] };
+    return { react: 'React', '@mdx-js/react': ['mdx'] };
 }
 exports.inferNamedImports = inferNamedImports;
 //# sourceMappingURL=imports.js.map
\ No newline at end of file

Additional context

Sorry for any mistakes. English is not my native language

@brillout
Copy link
Owner

PR welcome

@aleclarson
Copy link
Collaborator

For monorepo projects, it is likely that both react and preact packages required in the dependencies

Why is that likely?

@0xDing
Copy link
Author

0xDing commented Oct 21, 2021

For monorepo projects, it is likely that both react and preact packages required in the dependencies

Why is that likely?

We use (@uppy/react)[https://uppy.io/] in our projects, and preact is a dependency of @uppy/core

@silvenon
Copy link
Collaborator

@0xDing is using MDX v2 through @mdx-js/rollup an option for you? If yes, it has an option called jsxImportSource where you can specify preact.

@pzuraq
Copy link

pzuraq commented Mar 22, 2022

We are also using Uppy and encountering this issue in a Svelte application, despite not actually using Preact at all (we don't use the Preact rendering option). Inferring behavior based on what happens to be in node_modules is generally a bad idea, any package could decide to use React or Preact for rendering a component and that would mess up this build. Storybook is another example, it's used to document many frameworks but all of its components are React.

I think it'd make sense to stop inferring entirely and require this to be passed in, and release a new major. Would be happy to PR that if it sounds good @brillout.

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

5 participants