Closed jedwards1211 closed 4 years ago
No bundle size changes comparing 39c81703d0ab50f22db1be088eedc0b1facb0ecd...d7418695064c7f6537c704567b922355692164e4
Generated by :no_entry_sign: dangerJS against d7418695064c7f6537c704567b922355692164e4
@jedwards1211 Thanks for sharing. This is a pretty exciting path to explore and raise an interesting question: How can we improve the DX, down to the code editor?
However, I think that it's too early to promote it here. I think that the best approach would be to see how it perform (as evaluated by the community, not the core team), then we can give it further exposure. A few notes:
Actually someone made this for code snippets, but I haven't tried it: https://marketplace.visualstudio.com/items?itemName=Arunagiritm.material-ui Somehow I haven't been tempted to try it because I doubt it would work well for things like adding a dialog into an existing component...
I should provide a better demo of the Box codemod...the tedious thing when using @material-ui/system
is you have to figure out which style functions to import and compose together in the declaration of Box
to support which props you're using. The Box codemod does that automatically for you.
I'll work on making the README clearer, there's a before and after in this subproject: https://github.com/jedwards1211/material-ui-codemorphs#example
I'm sorry but I got the import path for Theme
from your own docs:
https://material-ui.com/guides/typescript/#customization-of-theme
Is there an alternative path I can import it from? Since it's a type import I assume it doesn't really matter how deep the path is?
When using system, do you typically just stuff all the style function imports you need for the entire project in one module and then import that wherever you need it?
I figured that with code splitting some chunks might be smaller if each module that uses system imports only the style functions it needs.
@oliviertassinari okay, I fixed the import paths now, looks like there's no problem with importing Theme
and WithStyles
from @material-ui/core/styles
. I guess we should update the docs though
Thanks for raising the issue with the docs. I think that we need to update the TypeScript's imports.
Regarding the Box, the tree shaking win is negligible. I think that you will be better off optimizing the cost of changes.
Closing per the initial concern: let's have the community to weight in.
What do you mean by optimizing the cost of changes?
What happens if you want to use a new prop that isn't available? You will need to rerun the codemod.
Right, but no one starts off by using every single style function anyway, do they? Running the codemod takes less than a second, you can even assign a keyboard shortcut to it. I think you overestimate the effort required to use it. The extension doesn't run jscodeshift on your entire project, it just runs it on the current text editor.
Try the Box codemod out and see how it feels to actually use it. It frees you from ever looking up which style function goes with which properties ever again, which is something you'd still have to do once in awhile if you compose all the style functions together in a single place in your project.
I don't think the community will actually be able to discover this and try it out unless it's publicized somehow...would it be too much to ask you to mention it on Twitter if I can't get a link in the related projects?
Right, but no one starts off by using every single style function anyway, do they?
I think that most do, at least when they use the Box. I have personally never imported anything from @material-ui/system
in a project.
I don't think the community will actually be able to discover this and try it out unless it's publicized somehow.
The Bootstrap community was able to discover the VS code extensions without linking it to the documentation. Same thing for Vuetify. For a UI library, I think that developers will be most interested in snippets. However, it might inspire others, so why not!
Regarding the withStyles codemod, it's pretty cool. Every time I have to add styles to a component, I feel friction, no more :).
@jedwards1211 What about the following action plan? :)
That sounds good, I can work on that in the next few days
Okay looking at @tmarun's snippets I see some problems:
onClose
, followed directly by a JSX element - these wouldn't be very convenient to expand anywheremui-
. I feel on the fence about whether this is necessary. It's kind of a shame that there's no way to configure this in a snippets extension. They should have just made a JS API for defining snippets. The limitations of JSON for defining snippets are a big mistake.How about I make a fork that addresses these issues? I'd also like to make the project structure more maintainable. (The JSON syntax for multiline snippets is truly awful, I'd rather define the snippets in JS files and have a build task output the final JSON for VSCode to use)
@jedwards1211 Thanks for the feedback on the snippet extension. The point regarding TypeScript is interesting. It seems that the withStyles codemod fails on a JavaScript file:
Regarding the prefix, I have seen Bootstrap prefix it with b4-
, Vuetify with v-
, Ionic with i-
. Ant Design with ant
. What do you have in mind?
Regarding forking, I don't think that Material-UI should officially support any extension, but if you have interests in pushing this further, sure, the better quality the extension available, the better for the community :)! Maybe you could find ways to join forces with @tmarun. No matter what, I wonder if developers would prefer different extensions for each concerns or a single one.
Hmmm, the problem is it's somehow not picking up your babel config, my extension requires @babel/core
from the project directory and uses it so that your project babel config is supposed to be picked up, but for some reason it's not working. I'll look into it...
If it helps, the codemod runs correctly with the tsx version of the same demo.
@oliviertassinari right, the problem is jscodeshift
's babylon
parser doesn't use the project's local babel config like it should, it just uses a default config. I'm trying to work around that in my code, but I think the problem might be that VSCode/Electron does some kind of bundling where you can't just require modules from arbitrary folders (shame)
@oliviertassinari okay, looks like the actual problem is require.resolve('someModule', {paths: ['/project']")
doesn't work inside a VSCode extension, it doesn't actually honor the paths
option like node does.
@jedwards1211 Off-topic, but I've had the same problem with eslint and prettier VSCode extensions ignoring the MUI config files.
@oliviertassinari okay, I fixed the issues and successfully got withStyles
to work on FreeSolo.js
! Also made sure to put the dash in "Material-UI" in the displayName
of the project.
@mbrookes huh, that's too bad. I've definitely noticed eslint acting up. I don't guess in the Prettier extension settings you have a static "Prettier: Config Path" set? You could also try checking the "Require Config" setting and see if it actually formats when you try to. As far as I can tell, the extension uses prettier.resolveConfig
, which I would think completely works, but maybe there's some issue with detecting prettier.config.js
files?
@mbrookes looks like prettier/resolve-config.js
uses the resolve
npm package, which...has a lot of code, but I'm assuming doesn't suffer the same issues as require.resolve
in Electron/VSCode, because it's a from-scratch implementation.
I had forgotten about makeStyles
, I need to make a codemod for that too
@jedwards1211 Thanks for looking in to it, and sorry for sending you off on a tangent! I had uninstalled it, but only ever had the default settings. Perhaps I'll give it another go. I might have been incorrectly blaming the prettier plugin for eslint plugin issues.
I'm going to mark these message as OT, just to clean up the main discussion, not because I don't appreciate your input!
Ouch...I spent several hours trying to debug why the Box
codemod wasn't working on files within the material-ui
repo itself, only to discover that resolve
was resolving @material-ui/system
to packages/material-ui-system/src/index.js
.
So just FYI, don't be surprised if Box
fails on a file within your own repo, I don't think there's a reasonable way I can make it work. It has to be able to require
the transpiled version of @material-ui/system
(to get style function metadata), which will be no problem for any project that depends on it.
@jedwards1211 Regarding the Box, I would propose no to support this codemod. My conviction is that developers will be better of including all the style functions (import { Box } from @material-ui/core
). The bundle size overhead is limited compared to the DX overhead of keeping the style functions import in sync with the props usage. But I appreciate the care you have put into it! Thanks for that.
We plan to include the system in the core components for v5.
Huh, I didn't even realize that Box is exported from core like that, I discovered it reading about @material-ui/system
. It seems kind of pointless to have individual style functions (and documentation that shows using them specifically) if there's no need to be so granular? Unless the idea was that people would compose their own custom style functions with them?
FWIW, I made the withStyles
(and the new useStyles
codemod, which is in CI right now) able to fall back to @material-ui/styles
if @material-ui/core
isn't present, just in case anyone is only using styles.
About snippets again -- a lot of @tmarun's snippets go ahead and include a className
prop, but I think that's not ideal for general use because anyone who doesn't need the className
prop would then have to delete it. I'm thinking I'll mostly stick to required props...the only exceptions I can think of at the moment are variant
, color
and anchorOrigin
/transformOrigin
.
if there's no need to be so granular
@jedwards1211 The motivation is to allow individual usage when authoring components, e.g. https://github.com/primer/components/blob/44444da23d76ff3445b899680f1be6dd8d027df2/src/Avatar.js#L24 => to better control the exposed API.
I see. It's ironic because I always wish people were less afraid of having lodash
as a dependency if they just need one function from it 😄
@oliviertassinari as I'm working on snippets, I'm thinking...users have different needs that are hard to support in a single extension. For example, do I make the snippets for form controls controlled or uncontrolled? Or do I provide both with -controlled
/-uncontrolled
in the snippet name? (ugh, too long...)
To make matters worse, it's not currently possible for users to override/customize the snippets coming from an extension: https://github.com/Microsoft/vscode/issues/10565
I'm almost tempted to make commands instead that insert the desired text based upon the preferences the user has selected in the extension settings. What do you think?
Also thanks for rating my extension!
Here are my WIP snippets: https://github.com/vscodeshift/material-ui-snippets/tree/master/src/snippets/javascriptreact
Feedback welcome, there are a lot of tricky decisions to make.
Obviously I'll need a folder for typescriptreact
too eventually.
@jedwards1211 It's pretty cool to see these snippets come to life. I might want to migrate to VSCode for them 😆. Maybe we should have two extensions, one for JS and another for TS? Right, it seems that it's what you have started to take as a direction.
I found out that VSCode has a Completions API for extensions, which is lower level, so I could give the same experience as snippets, but with the ability to toggle options in the extension settings :)
I'm trying to decide how I want to structure the JS/TS dichotomy, I'm tempted to have them in the same project because I think most of the snippets will probably be exactly the same. Also if I use the Completions API, I could probably have it add any missing imports that are needed for a snippet once it's selected.
I just realized though, I had to suppress the intellisense because the buggy VSCodeVim extension is super slow when intellisense is enabled. And unfortunately it prevents snippets from appearing in the command-space menu, I'll have to see about completions. I might enough up having to make commands to insert "snippets" as a backup.
Okay I just published a first version of the snippets: https://marketplace.visualstudio.com/items?itemName=vscodeshift.material-ui-snippets
It definitely doesn't have everything yet, and it automatically adds imports, but only if the file has no syntax errors (I'd like it make it tolerate some syntax errors so that it still works if someone uses a template in the middle of typing up a statement).
@jedwards1211 I can't make it work :/. I have "editor.tabCompletion": "on"
but it doesn't seem enough nor with ⌃Space.
Okay, you were in TypeScript react or JavaScript react mode?
I haven't confirmed but I think the completion API doesn't work exactly the same as snippets. Typing the snippet name and hitting tab may not work. But if you press control space and wait for the menu to load, then start typing mui-
you should see the completions come up if the extension is working. Let me know if that doesn't work
Oh actually maybe I just need triggerCharacters: ['\t']
where I register the completions.
Okay, you were in TypeScript react or JavaScript react mode?
I have no idea. I have tried with a file in JavaScript and another in TypeScript. Do I need to configure something or does it use the file's extension?
Kendo has recently announced a scaffolding extension https://youtu.be/4N5ky1mvTgg?t=2035, but it doesn't seems to get more usage than the none offcial snippet plugin https://marketplace.visualstudio.com/search?term=Kendo&target=VSCode&category=All%20categories&sortBy=Relevance.
@oliviertassinari okay, the installed extension isn't working for me like when I was running it in the debugger, sorry, I should have checked. Gotta figure out how to debug an installed extension. I hate VSCode's extension host architecture...it runs the extensions in separate processes to prevent them from slowing down the UI (which is a joke, the VSCodeVim extension causes all kinds of lag in the UI), and it's more trouble than it's worth, I don't even know how to debug an installed extension atm.
Alright, I fixed the issues with the extension, feel free to upgrade and try it out now
@jedwards1211 Oh boy, it starts to take shape, it's pretty exciting! 😍 I could make it work with TypeScript. I would suggest the following area of improvements:
mui-tooltip
.I have created a publisher account for Material-UI, in case of it can be useful.
Huh did you try in a .js
or .jsx
file? If just .js
, you'll have to click on JavaScript
in the bottom right hand corner and manually change the language to JavaScript React
. The completions work for me in that mode.
Maybe I should just make it provide the completions for plain JavaScript
as well, since a lot of people like me use .js
for our React files. Or I could explain in the README how to tell VSCode to treat all .js
files as JavaScript React
. What do you think?
you'll have to click on JavaScript in the bottom right-hand corner and manually change the language to JavaScript React. The completions work for me in that mode.
@jedwards1211 You are right. You spot me, I'm not a regular user of VS code. I have tried with a .js file. It does work 👌 once I set the right language. A note about it could help noobies like me 😆.
I think VSCode users out there will really love this extension I just made!
It has commands for wrapping a component in the text editor in
withStyles
, and another for automatically creating/updating up the declaration ofBox
for work with@material-ui/system
.https://marketplace.visualstudio.com/items?itemName=vscodeshift.material-ui-codemorphs