[core] Remove createSvgIcon duplication #20308
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Details of bundle changes.Comparing: e9fbcbf...e17db10 Details of page changes
|
Member
Author
|
@dmtrKovalenko No matter the outcome of this pull request, we will need to remove the imports above 2 layers, e.g: import createSvgIcon from '@material-ui/core/internal/svg-icons/createSvgIcon';As far as I know, this means that developers could bundle the styles twice (1 ESM, 1 CJS), leading to bundle bloat and incorrect CSS injection order. |
eps1lon
reviewed
Mar 28, 2020
Member
There was a problem hiding this comment.
Can we move this into core/src/utils instead? I don't think this tiny helper warrants its own module.
Still think that making this public is a bad idea. We're not creating a SVGIcon but a "Material-UI SVG icon". People will want us to support all sorts of different svg build scenarios.
Member
Author
Only the modules available at |
oliviertassinari
force-pushed
the
share-internal-createSvgIcon
branch
from
c8a2961 to
f1fb94d
Compare
oliviertassinari
force-pushed
the
share-internal-createSvgIcon
branch
from
f1fb94d to
e17db10
Compare
oliviertassinari
added
the
internal
EsoterikStare
pushed a commit
to EsoterikStare/material-ui
that referenced
this pull request
Mar 30, 2020
Contributor
|
This breaks the latest version of |
Collaborator
|
@koistya which version of pickers are you using? |
This was referenced Apr 6, 2020
|
does this impact @material-ui/lab? I recently bumped dependencies, "version": "4.0.0-alpha.48", and "version": "4.9.9", and when I run jest tests I am getting TypeError: (0 , _utils.createSvgIcon) is not a function when I include any ideas? |
|
getting this when I try and run the app: warning in ./node_modules/@material-ui/lab/esm/internal/svg-icons/FirstPage.js "export 'createSvgIcon' was not found in '@material-ui/core/utils' |
Contributor
|
@koistya, @kelly-tock are you able to confirm that you only have 1 copy of |
Member
Author
|
@kelly-tock Upgrade your dependencies, you will be fine. |
Contributor
|
@oliviertassinari from what I can tell, @kelly-tock is already using the latest |
|
ok, yarn messed with me. sorry about that. I removed node modules, cleared yarn cache. specifically did 4.9.9. and now its fine :) |
|
out of curiosity, do you all prefer yarn or npm? |
Contributor
I personally prefer Yarn and find I'm not sure if this is what you did, but I would recommend against blowing away your lockfile because that typically should be a last resort, especially if you have a large dependency tree - you could introduce lots of subtle changes that cumulatively break your project and it would be hard to diagnose due to the size of the change. Instead, per https://material-ui.com/getting-started/faq/#why-arent-my-components-rendering-correctly-in-production-builds
you could either have another place in your project where you didn't update |
This was referenced Apr 13, 2020
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A follow-up on #20268.