Closed aplaskota closed 2 years ago
This sounds very much like https://github.com/adazzle/react-data-grid/issues/1999. Is there any of the explored direction that catches your eye?
yep, we have different root cause but indeed we're going in the same direction. I believe solution no 1 that you suggested is as simple and fast as possible. I'm opting for that.
import Clear from '@material-ui/icons/Clear';
function ClearIcon() {
return <svg viewBox="0 0 24 24" style={someStyles}>{Clear.path}</svg>;
}
@aplaskota I'm not sure how we can implement it effectively. It seems that we would need to split the source. Maybe
import clear from '@material-ui/icons/md/Clear.path';
function ClearIcon() {
return <svg viewBox="0 0 24 24" style={someStyles}>{clear}</svg>;
}
?
@oliviertassinari I think we can hold on to waiting for more followers of that idea. It seems that I'm the only one who requested that change. Actually I can copy and paste some icons into my internal component
what about making the path
exported var and default export leave as it is?
in this case refactoring only affects the internals, everything else stays the same. Since the core
is already in the peerDeps
this shouldn't be a big problem. Time consuming yes, but not complex.
@woss So you are advocating for, using one icon as an example:
diff --git a/packages/material-ui-icons/src/Info.js b/packages/material-ui-icons/src/Info.js
index 27b35d5bf2..c6e6f2d7d8 100644
--- a/packages/material-ui-icons/src/Info.js
+++ b/packages/material-ui-icons/src/Info.js
@@ -1,6 +1,6 @@
import * as React from 'react';
import createSvgIcon from './utils/createSvgIcon';
-export default createSvgIcon(
- <path d="M12 2C6.48 2 2 6.48 2 12s4.48 10 10 10 10-4.48 10-10S17.52 2 12 2zm1 15h-2v-6h2v6zm0-8h-2V7h2v2z" />
-, 'Info');
+export const path = <path d="M12 2C6.48 2 2 6.48 2 12s4.48 10 10 10 10-4.48 10-10S17.52 2 12 2zm1 15h-2v-6h2v6zm0-8h-2V7h2v2z" />;
+
+export default createSvgIcon(path, 'Info');
The implementation should be as simple as:
diff --git a/packages/material-ui-icons/templateSvgIcon.js b/packages/material-ui-icons/templateSvgIcon.js
index 40232b111f..e87a42305d 100644
--- a/packages/material-ui-icons/templateSvgIcon.js
+++ b/packages/material-ui-icons/templateSvgIcon.js
@@ -1,6 +1,6 @@
import * as React from 'react';
import createSvgIcon from './utils/createSvgIcon';
-export default createSvgIcon(
- {{{paths}}}
-, '{{componentName}}');
+export const path = {{{paths}}};
+
+export default createSvgIcon(path, '{{componentName}}');
If this allows tree shaking, I think that it's worth trying out.
If it doesn't, solving the problem with:
diff --git a/packages/material-ui/src/utils/createSvgIcon.js b/packages/material-ui/src/utils/createSvgIcon.js
index 2d146ab87d..dc805367f4 100644
--- a/packages/material-ui/src/utils/createSvgIcon.js
+++ b/packages/material-ui/src/utils/createSvgIcon.js
@@ -17,6 +17,8 @@ export default function createSvgIcon(path, displayName) {
Component.displayName = `${displayName}Icon`;
}
+ Component.path = path;
+
Component.muiName = SvgIcon.muiName;
return React.memo(React.forwardRef(Component));
could be even simpler.
I have tried to advance the reflection from https://github.com/adazzle/react-data-grid/issues/1999. From what I understand, we have these options:
@material-ui/core
for nothing, especially for a library. But to be fair a library would probably not install a full list of 5,000 icons for using 5.@eps1lon A related question on the size of the icons repository. Do you know when would we be able to drop the .cjs extension?
I'm also wondering. Do we really need to support `import { Info } from '@material-ui/icons';
yeah, exactly that. The implementation looks simple and you don't have to create any new files. :)
As for the simpler
solution, i cannot comment since i am not super familiar with the React.memo
@woss I assume by "that", you are referring to option 1. Would you mind sharing more about your use case for using only the SVG path? Thanks
@oliviertassinari yep, option 1.
as for your question i guess you would like to know how would we use the path
exactly?
this is the grommet code for one icon
import { StyledIcon } from '../StyledIcon';
export const Accessibility = props => (
<StyledIcon viewBox='0 0 24 24' a11yTitle='Accessibility' {...props}>
<path fill='none' stroke='#000' strokeLinecap='round' strokeLinejoin='round' strokeWidth='2' d='M4,8 L11,8 L11,14 L7,21 M20,8 L13,8 L13,14 L17,21 M12,5 C12.5522847,5 13,4.55228475 13,4 C13,3.44771525 12.5522847,3 12,3 C11.4477153,3 11,3.44771525 11,4 C11,4.55228475 11.4477153,5 12,5 Z M11,8 L13,8 L13,13 L11,13 L11,8 Z' />
</StyledIcon>
);
So i guess we would make the Wrapper Component to use the StyledIcon
similar to this where path
would be a prop then use it like
<SvgIcon path={path}/>
I hope i answered your question.
@woss Would it be OK if you still have to install @material-ui/core (what option 1 implies, as far as I know)?
Do you know when would we be able to drop the .cjs extension?
As far as I remember we only use .js extensions for shipped files due to lacking tooling support. As to not shipping a commonjs build: Once we drop support for node 10. Node 10 reaches EOL on 2021-04-30.
@oliviertassinari well i guess installing no, but bundling it yes. not sure atm, i'll have to check with my guys and tell you more after that
on the other hand i got the idea to completely extract paths into the separate folder called paths
inside the packages/material-ui-icons
( for the time being and for the discussion) @oliviertassinari check my fork and implementation here https://github.com/woss/material-ui/tree/woss-extract-paths/packages/material-ui-icons/paths
If we go this way, you could create separate repo just for the paths, then include that into the icons and the rest of us can just install the paths and have our implementations.
Later when google introduces new icons, or changes the current ones, just update this new repo and voilà we all benefit and get then new updated icons without any overhead.
Please tell me what do you think
@eps1lon Ok thanks. It sounds like we will be able to make the icons repository lighter in the near future. @woss So you are suggesting option 2 or 3 from https://github.com/mui-org/material-ui/issues/21251#issuecomment-688923161?
With this approach option 3 would be the best, don't you think?
Option 2 with the new files that i've created would be a good solution but you would still need to do the same thing as in option 3, difference would be that with option 3 material-io/core is not installed even declared since the files are just strings and it can be used anywhere.
I understand that it requires time to set it up but honestly i've spent 30 min creating these files, mostly went on the how to sed this and that
.
Option 3: when you make the new package iconPaths
or however you want to call it, let me know and i will add the files from my fork to it. then we PR and merge upon the approval, you publish it and i get to install them and get rid of bloated package we are using now :)
@woss If I understand: a. you don't want to have @material-ui/core bundled, why? b. you don't want @material-ui/core to be installed in your file system, why?
Note that with option 2, we can make @material-ui/core an optional peer dependency, which will solve both a. and b.
@oliviertassinari
a: i really don't need it b: i really don't need it
Why creating new package that will hold only the svg paths and nothing else doesn't sound like a great approach?
@woss If all you want are the raw SVG paths, you might be better off starting here (Although it seems that whoever at Google is now maintaining this scrapes them! 😆)
I'd also really like a standalone @material-ui/icons
package. Having to install the entire @material-ui/core
package for a single function createSvgIcon
seems pretty crazy to me. Whilst I understand this sits on the file system and doesn't get built into production with tree shaking, it still takes up time locally and on CI installing. And it's confusing for new developers reading our package "why do we have this? aren't we using x framework instead?" And the perfectionist in me can't stand having a dependency we don't directly use 🥲
Further, here's an analysis of a create-react-app bundled app that imports just 3 icons from @material-ui/icons
directly (like this: import AccountBalanceWalletOutlinedIcon from "@material-ui/icons/AccountBalanceWalletOutlined";
)
19.5kb for three icons is pretty crazy.
19.5kb for three icons is pretty crazy.
The library is not intended to be used with just 3 icons. You should download the icons directly from https://material.io/icons/
I'm closing as the issue didn't get much interest since it has been open.
19.5kb for three icons
With v5.0.0-beta.2, the pain is reduced. The overhead of one icon is now about the layer we add on top of styled-components/emotion. Mainly the system and the default theme. With a bare Next.js installation:
The overhead seems to be 6+ kB gzipped now, assuming you would already have emotion or styled-components in your bundle.
It's really a shame that v5 requires emotion, we don't want extra styles runtime lib so we'll stick to v4. 🤷♂️
Yeah, would indeed be nice to have this as standalone package.
Summary 💡
I'd like to use in my react project only material icons without need to install other package
Motivation 🔦
It may be convenient in case someone would like to use only material icons