[legacy-framework] Add material-ui recipe

[drenther]

Closes: None

What are the changes and their implications?

Adds a recipe for the Material-UI. Most of the code is similar to the chakra-ui recipe with the only exception being an empty initial theme object variable declaration similar to what is in the material-ui docs ThemeProvider example. That can be used to extend the theme as the user sees fit in the future.

Note:
The TODOs and improvements suggested in the chakra ui recipe like this haven't made it into the installer so that bit is still copy-pasted (can by cleaned up in a future PR post the changes?) -

@aem Some notes and improvements for the installers in general

Most installers will interact with pages / _app / _document, and since they're are export default, there should be a super simple and reusable function to find that AST node
Also most installers will wrap App in a provider (Random stores, Themes...), so we should also have a reusable function to do so

Checklist

• Tests added for changes
• PR submitted to blitzjs.com for any user facing changes

[flybayer]

Thank you @drenther! I haven't tested this, but initially it looks good.

Do you have more work to do on it? If not, you can mark is as "Ready for review"

[drenther]

Hey @flybayer, I initially started the implementation naively assuming it will work as it is used in a client side only app. On testing it didn't. So now making changes according to this example that works for next.js - https://github.com/mui-org/material-ui/tree/17b9a90bf8488d1a96fbb2f6528eb0c30308ce2e/examples/nextjs

I have committed the changes for customizing _app.ts and adding theme.ts. The one left to add is the _document.ts where I need to customize the getInitialProps. Should be ready by today or at max tomorrow.

[gabrielcolson]

Hey @drenther!

I tried to manually add Material-UI to a blitz app following the nextjs example like you did in this recipe. The problem is that Material-UI does not yet support strict mode. I ran into issues when I tried to use components that use dynamic styling like Box.

If there is a workaround (I didn't find any for now), maybe it should be included in this recipe. If not, we could add it to the documentation to warn the users.

[drenther]

@gabrielcolson Thanks. I followed the trail of the material-ui examples PR when writing this PR. I am trying to add a workaround for it and reasoning for it in the comments. Also, let the user know during installing of recipe that this path is currently React.Strict incompatible.

[flybayer]

Since it's not strict mode compatible, that means you need to add this to blitz.config.js to disable concurrent mode which is enabled by default.

module.exports = {
  experimental: {
    reactMode: "legacy"
  }
}

Also, each time you use useQuery you need to set suspense: false and then use the isLoading, error, etc items returned from useQuery for showing loading and error states.

const [stuff, {isLoading, isSuccess, error}] = useQuery(getStuff, {id: 1}, {suspense:false})

[drenther]

The suspense bit will need to be mentioned in the docs and when installing the recipe?

[flybayer]

I think just in the recipe installation. Perhaps we could create a github discussion thread on material UI and link to it as a place for workarounds and help.

[gabrielcolson]

Forcing the removal of React Suspense by default in the recipe seems overkill as it only affects the Box component and dynamic styling of @material-ui/styles. Those are very minor features of Material UI.

Maybe it would be sufficient to only warn the users about it and point them to this issue.

[drenther]

@gabrielcolson That's a good point. By dynamic styling do you mean customizing the theme passed to the ThemeProvider or use of APIs like makeStyles and/or withStyles. Because these APIs are quite commonly used from what I have seen in codebases using @material-ui.

[gabrielcolson]

From what I understand it is only when passing parameters to the hook generated by makeStyles (which is pretty uncommon). I am currently building an app with Blitz and Material UI and I didn't encounter any issues except for the Box component which is the only component that uses dynamic styling internally.

[drenther]

Looks like it would be better if we skipped the legacy mode bit and added a warning note in the recipe installer notifying the user about strict mode incompatibility when using Box and/or dynamic styling features as you mentioned. @flybayer what's the call here?

[drenther]

Pushed all the changes required to get the recipe running. Tested it out and seems to work fine with Box component and all as well (since the legacy mode switch is currently on). Let me know if need to roll that back. Opening for review now.

[flybayer]

Oh, good to know that you only need legacy mode for those certain cases. I was under the impression it was needed for everything.

In this case, let's remove legacy mode from the recipe and instead add a warning about those cases.

Thanks everyone!

[aem]

I'm gonna update the addImport function we currently expose to take just a module name and a ImportSpecifier | ImportDefaultSpecifier so we can hide all of this logic away. It's fine to merge this as-is if we want, but otherwise we can remove a lot of this boilerplate

[aem]

this is another thing that my refactor branch helps with - we'll be able to pass in a template string with valid TS in it, and the output will be the right AST nodes. might be worth waiting for that change to land

[drenther]

@aem The refactor branch sounds good. Will reduce a lot of boilerplate and make this recipe code easier to reason with.

@flybayer @gabrielcolson I have removed the legacy mode transform in the blitz.config.js file and added an extensive (may be too big, may need some help trimming it down and making it more digestable.)

Let me know if anything needs to done on this.

[flybayer]

Awesome, thank you @drenther!!

@all-contributors add @drenther for code

[allcontributors]

@flybayer

I've put up a pull request to add @drenther! 🎉