feat: added ability to inject style tags at custom point in head

[magnusriga]

also updated README to explain how it is done

closes #19

[magnusriga]

@hupe1980 @oliviertassinari I implemented Oliviers suggestion. You can now specify the options in a separate config file, including original jss options. The jss options will be extracted and passed on to the jss.create(options) function. styles/jssPreset() is also included, and only overwritten if own options are specified.

injectFirst takes precedence over insertionPoint / noscript properties, if specified. Tested with both options on and off, seems to work well.

Thoughts?

[hupe1980]

@magnusriga @oliviertassinari I thought about that again My two cents:

Maybe we should not support every conceivable case and keep the plugin simpler:

"injectFirst" covers most of the cases for me, since many use Material-UI together with styled-components. For other cases we could still allow you to give a special insertion point. You would have to insert the point in a custom html.js, for example. We do not need solutions for "Create React App strips HTML comments", "codesandbox.io prevents access to the element" anyway.

I would like it if you can change the productionPrefix, since I've already used it myself. I had it in the old plugin

I can not say much about jss-plugins, as I have not used this case so far.

When it is rarely needed, my suggestion for a signature would be:

resolve: `gatsby-plugin-material-ui`,
options: {
        // disableAutoprefixing: false
        // disableMinification: false
        stylesProvider: {
           injectFirst: true,
           // insertionPoint: 'custom-point',
           productionPrefix: 'c'
        },
      },

The cases that we do not support could then be listed in the readme and recommend the creation of a local plugin

[oliviertassinari]

can not say much about jss-plugins, as I have not used this case so far.

@hupe1980 jss plugins are important for right-to-left support.

IMHO, the simplest API is to allow the serialization of StyleProviders options, e.g. injectFirst as well as the non-serialization with a config file, e.g. for plugins, class name generator, custom JSS instance. We might want to add an insertionPoint option in StyleProviders directly rather than here.

[magnusriga]

@hupe1980 @oliviertassinari I believe it is important in Gatsby, to be able to insert it where the noscript tag sits (because their webpack strips away html comments)? I built a plugin (gatsby-plugin-global-styles) as well as a small library (@nfront/gatsby-plugin) to be able to inject global styles at the desired point in head. When using MUI together with styled-components, global-styles, typography.js, and more, it would be critical to be able to set its injection point programatically with noscript.

I think it's a good point though, to keep the plugin simple to use (but still supporting noscript)

[magnusriga]

@oliviertassinari what would be your recommended full signature for the 1) Plugin (gatsby-config.js), and 2) gatsby-mui-config.js ?

If we land on a solution, I can update the PR.

[oliviertassinari]

@magnusriga I would propose:

module.exports = {
  plugins: [
    {
      resolve: `gatsby-plugin-material-ui`,
      // If you want to use styled components, in conjunction to Material-UI, you should:
      // - Change the injection order
      // - Add the plugin
      options: {
        disableAutoprefixing: true,
        disableMinification: true,
        stylesProvider: { // <StylesProvider /> props
          injectFirst: true,
        },
        // pathToStylesProvider: 'src/utils/gatsby-mui-config',
      },
      // 'gatsby-plugin-styled-components',
    },
  ],
};

where pathToStylesProvider JavaScript file supports the same props as stylesProvider, it's only here to avoid the serialization issue. I think that we can forbid people to the two property at the same time (stylesProvider and pathToStylesProvider).

In the future, we can add an insertionPoint prop to the <StylesProvider /> component so you can do:

module.exports = {
  plugins: [
    {
      resolve: `gatsby-plugin-material-ui`,
      options: {
        stylesProvider: { // <StylesProvider /> props
          insertionPoint: '#jss-insertion-point',
        },
      },
    },
  ],
};

What do you think about it?

[magnusriga]

@oliviertassinari Thanks for the input. A couple of follow-up questions:

1) I might be missing something, but how does your suggestion support a custom injection point based on a noscript tag? As it stands, how will gatsby-browser.js know if the insertionPoint is to be interpreted as a HTML comment or as a noscript tag?

2) What is the serialization issue?

PS: Just for context, my initial goal was to support a custom injection point for mui in Gatsby (since I use many style tag providers, including typography and global-styles).

Thank you,

Magnus

[oliviertassinari]

1. You would need to insert your own noscript in the head element with an id and to provide a pathToStylesProvider file returning the JSS instance configured with the correct insertion point. It's not the most direct approach, but at least, it supports all the cases. Then, we can think of simplifications.
2. I believe Gatsby only accepts serializable objects in its option configuration file. For instance, you can return a function.

[magnusriga]

@oliviertassinari Think we can hop on a quick call to discuss? It would be more efficient. What you describe in (1) is what I was already doing, but I don't see how we can do document.querySelector in any other file but gatsby-browser? The document object needs to be present.

PS: I dropped you an email a couple of days ago, on the address you gave me. Did it make it through to you?

[magnusriga]

@hupe1980 Just to keep you in the loop, I spoke with Olivier this morning to better understand his vision.

Correct me if I am wrong @oliviertassinari , but in summary:

1) We allow the user to specify the props belonging to StylesProvider, including injectFirst, jss, disableGeneration, generateClassName, serverGenerateClassName, sheetsCache, sheetsManager, sheetsRegistry. @oliviertassinari, do we need to support all of those? 2) We allow the user to specify those, either directly in gatsby-config.js, OR via a separate js file (gatsby-mui-config.js) which the user points to with the pathToStylesProvider option in gatsby-config.js. We note in the README that only ONE of the two approaches are allowed at the same time. 3) The purpose of supporting a separate config file (gatsby-mui-config.js), is so that we can create an actual jss instance within the config file itself (unlike in gatsby-browser.js, like I originally did). 4) By allowing the user to create the jss instance, we can support an insertionPoint either as a HTML comment, or as a DOM node (because the cached file will be imported and ran in gatsby-browser.js).

With the above approach, the plugin should support all required scenarios.

As a next step (not part of the plugin), we implement the actual insertionPoint as a property on the @material-ui/styles/StylesProvider. That way, we can remove StylesProvider's jss dependency altogether. Did I understand that part correctly, @oliviertassinari ? What about the RTL support you spoke about? I am not sure I got what that was...

If that sounds OK to you both, I will finish of the plugin part first.

[oliviertassinari]

@magnusriga Is sounds right to me.

1. The plugin implementation doesn't need to know the existence of these props as long as it forwards them. I think that it's important to forward all of them, so we don't create any roadblock for people to experiment.
2. I would even throw to prevent it, on top of the documentation note.
3. Yes, or a custom class name generator function, a new JSS plugin, etc.

[magnusriga]

@hupe1980 @oliviertassinari I made the discussed changes.

Want to review and merge if it looks ok?

[oliviertassinari]

@hupe1980 How do you test the changes in a real project? I'm not sure how I should proceed. The best option I can think of is to duplicate the code.

[magnusriga]

Hi @oliviertassinari . I did not add any tests, but instead tested it through usage. Do this:

1) clone the repo 2) npm install 3) npm run build 4) Copy the 4 built files (gatsby-browser.js, gatsby-ssr.js, gatsby-node.js, index.js) as well as package.json to your local plugins folder (gatsby-default-starter/plugins/gatsby-plugins-material-ui) 5) Create src/utils/styles-provider-props.js to test specifying the props in a separate file (see the README I just committed).

[oliviertassinari]

I don't think that we can easily automate the tests. But at the same time, I wish we had a simple way to manually test it. A copy workflow is error prone. I'm wondering how Gatsby solves the problem.

[hupe1980]

@oliviertassinari How about a monorepo with plugin and gatsby application[s]. We could then someday automate tests with cypress: https://www.gatsbyjs.org/docs/end-to-end-testing/ But before that you can also test it manually

[oliviertassinari]

This sounds great!

[hupe1980]

I have created a PR #24

[magnusriga]

@oliviertassinari @hupe1980 I automated tests with Travis and Jest for another Gatsby plugin I wrote with similar characteristics as this one. Would that be easier than copying the whole Gatsby source into another repo? I could reuse most of the tests I think. It has 100% test coverage and would be fast / simple to copy.

You can see it live at GitHub: gatsby-plugin-global-styles and @nfront/global-styles

[oliviertassinari]

@magnusriga I think that we should go with an end-to-end test case like @hupe1980 is proposing in #24 rather than testing the logic in isolation. It's slower to run but it provides more guarantees.

[hupe1980]

@magnusriga @oliviertassinari I would do both. With unit tests, the following can be better tested:

• It should disable autoprefixing
• It should disable minification
• It should throw an exception if the pluginOptions provide injectFirst and insertionPoint at the same time
• ...

[oliviertassinari]

@magnusriga Could you rebase the pull request, we have an e2e test now :)?

[magnusriga]

Got it, re. both previous posts 👌🏼

I'll rebase.

[magnusriga]

Ok, I think I did the rebase correctly. I always get a bit confused by git, and I might have sneaked in some unnecessary commit messages.

PS: Beyond updating the plugin and its README, I also changed the prettier to single tick. Feel free to change it back if you prefer (I typically just follow the AirBnB standard).

[magnusriga]

@hupe1980 @oliviertassinari any update here gents?

Ready to merge the PR?

[oliviertassinari]

@magnusriga It looks good to me from a global perspective, well done! @hupe1980 might want to refine the details.

[magnusriga]

Linting errors seem to be all about the quotation rule (I went for single tick).

[magnusriga]

@hupe1980 In order to unit test the plugin files, you have to virtually mock the cache module(s). Here is an example in that other plugin I made: https://github.com/nfront/gatsby-plugin-global-styles/blob/master/src/__tests__/gatsby-browser.test.js

Let me know if you want me to do anything there. Happy to jump into the tests too.

[hupe1980]

@magnusriga I'll have a deeper look at the PR this week

[magnusriga]

@magnusriga Can you please also fix the tests?

Yep, but not sure exactly when I will have time. I have been using the modified plugin for over a month now, without issues. I wonder if we should merge it before we start striving for 100% test coverage?

[hupe1980]

@magnusriga Can you please also fix the tests?

Yep, but not sure exactly when I will have time. I have been using the modified plugin for over a month now, without issues. I wonder if we should merge it before we start striving for 100% test coverage?

We should update the existing tests so that they are green again:

@circleci-checks build-test Failing after 6m — Workflow: build-test Details

ci/circleci: lint — Your tests failed on CircleCI Details

ci/circleci: unit_tests — Your tests failed on CircleCI Details

[magnusriga]

@hupe1980 @oliviertassinari re-committed. Now passing all tests.

[magnusriga]

@hupe1980 @oliviertassinari Can we merge and publish this gents? It is working and I am getting requests on LinkedIn from people that want to use it.

[magnusriga]

I am commiting the changes now @oliviertassinari

[oliviertassinari]

@magnusriga What do you think of adding the following at the end of the markdown?

## Usage with styled-components

When using the core components of Material-UI with styled-components, you need to make sure the CSS injection order is correct.
You can configure it like this:

```js
module.exports = {
  plugins: [
    {
      resolve: 'gatsby-plugin-material-ui',
      options: {
        stylesProvider: {
          // Inject the CSS of Material-UI first in the <head>, before styled-components.
          injectFirst: true,
        },
      },
      'gatsby-plugin-styled-components',
    },
  ],
};

[magnusriga]

@oliviertassinari That code block is identical to the initial one though, where we explain how it works.

My suggestion is to change the how-to description so it says:

If using Material-UI together with other styling providers (like styled-components), you should make sure Material-UI styles end up on top of head (so the other styling providers can overwrite it). To ensure that, follow one of the two options:

1) Add a stylesProvider property, and specify injectFirst: true (see below). This places Material-UI at the top of head 2) Alternatively, specify a specific point in head where the styles will be injected. This is done by dropping the stylesProvider property, and instead specifying the pathToStylesProvder property. It should point to a configuration file (see below). You will find an example configuration file below.

[oliviertassinari]

@magnusriga Sounds great! For point 1., I would add a code example people can copy & paste in their Gatsby configuration file 👌

[magnusriga]

@magnusriga Sounds great! For point 1., I would add a code example people can copy & paste in their Gatsby configuration file 👌

Great. I will implement and push it later this evening.

[magnusriga]

@oliviertassinari @hupe1980 I tried to add a few things to the development-runtime in the monorepo, but now it does not want to run gatsby develop (or gatsby build`).

Any idea?

[oliviertassinari]

I'm continuing the pull request.

[oliviertassinari]

It's ready for review: https://github.com/hupe1980/gatsby-plugin-material-ui/pull/27 cc @hupe1980 and @magnusriga.

[hupe1980]

I'm out of office until the middle of next week. Then I will look at it