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

Fix Icon import in React #68

Closed
wants to merge 2 commits into from
Closed

Fix Icon import in React #68

wants to merge 2 commits into from

Conversation

HugoGresse
Copy link

Proposed Changes

This PR fix a bug happing in production build where the <Icon /> is React was not imported correctly resulting in this error:

Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: object.

Source: https://reactjs.org/docs/error-decoder.html/?invariant=130&args[]=object&args[]=

Types of Changes

What types of changes does your code introduce?

  • Documentation Update
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have read the Contributing doc.
  • Lint passes locally with my changes.
  • I have added necessary documentation (if appropriate).

Additional Information

Linked issue and PR:

@mririgoyen
Copy link
Member

I don't think this is a valid change. 🤔

Are you personally experiencing issues importing the Icon component in the way our documentation states to do so?

import Icon from '@mdi/react';

Icon is the default export from that package. We use that exact import statement all over this site without any issues. Are you by chance disabling default imports? Or have some other non-standard configuration that could be causing issues with imports? Could you give a little more details into this for reproduction?

@HugoGresse
Copy link
Author

HugoGresse commented Mar 2, 2023

I've setup a sample repo here:
https://github.com/HugoGresse/material-icon-vite-import-bug

Deployed here: https://hugogresse.github.io/material-icon-vite-import-bug/

I've not digged down into it but the sample only use Vite + TS + mdi package which should be a lot of project I guess. No specific setup for vite except the base path for the github pages, directly raw project.

Expected result: some text and a red icon rotating below:
ezgif-1-4233c9725f

@Templarian
Copy link
Collaborator

Templarian commented Mar 3, 2023

https://github.com/Templarian/MaterialDesign-React/blob/master/dist/Icon.d.ts#L5

Looking at the source code for the dist folder for the @mdi/react package. The default is Icon.

https://github.com/Templarian/MaterialDesign-React-Demo/blob/master/src/index.js <- The demo site for instance.

I haven't looked closely at repo that @HugoGresse put up, but my guess it's a configuration issue.

@mririgoyen
Copy link
Member

I've setup a sample repo here: https://github.com/HugoGresse/material-icon-vite-import-bug

I've pulled this repo down, installed, and npm run dev and I see the icon as in your screenshot as you would expect. It appears this only happens in a Vite production build.

A quick Google search immediately led me to vitejs/vite#4704, which is closed as a duplicate of vitejs/vite#2139, showing that this is a configuration problem with Vite. It seems someone has a workaround on this comment.

Since this isn't a problem on our side of things, I'm going to close this and the other related PRs and issues. Best of luck figuring out things with Vite!

@mririgoyen mririgoyen closed this Mar 3, 2023
@mririgoyen mririgoyen added Invalid Spam or Off-Topic Won't Fix This will not be worked on and removed Invalid Spam or Off-Topic labels Mar 3, 2023
@HugoGresse HugoGresse deleted the patch-1 branch March 3, 2023 12:57
@HugoGresse
Copy link
Author

thanks for digging into this, sorry for the noise!
Crazy this behavior is still here after 2y.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Won't Fix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants