Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

theme.styles are not applied in Gatsby site #1345

Closed
maiertech opened this issue Dec 8, 2020 · 10 comments
Closed

theme.styles are not applied in Gatsby site #1345

maiertech opened this issue Dec 8, 2020 · 10 comments
Labels
bug Something isn't working

Comments

@maiertech
Copy link

Describe the bug
I have a Gatsby site that does not apply any styles from theme.styles during MDX transformations with theme-ui and gatsby-plugin-theme-ui both at v0.3.4.

To Reproduce

  1. Clone https://github.com/maiertech/gatsby-themes/tree/theme-ui-styles-broken
  2. Run yarn
  3. Run yarn workspace digital-garden-example run dev
  4. Launch http://127.0.0.1:8000/posts/ut-officia-officia-ut-lorem-quis-deserunt-irure-nisi-culpa-aliqua-ex-lorem-magna/ and note that the two links are unstyled, even though theme.styles styles them.

Screen Shot 2020-12-08 at 1 07 00 AM

Expected behavior
Both links should not be styled with browser defaults but with primary color.

Additional context
This looks like #1148 posted by @cwgw.

Running yarn list --pattern mdx yields this:

yarn list v1.22.5
├─ @mdx-js/mdx@1.6.22
├─ @mdx-js/react@1.6.22
├─ @mdx-js/util@1.6.22
├─ @theme-ui/mdx@0.3.4
│  └─ @mdx-js/react@1.6.21
├─ babel-plugin-apply-mdx-type-prop@1.6.22
├─ gatsby-plugin-mdx@1.6.0
├─ gatsby-recipes@0.5.0
│  └─ remark-mdx@2.0.0-next.8
├─ remark-mdx@1.6.22
└─ remark-mdxjs@2.0.0-next.8
   └─ @mdx-js/util@2.0.0-next.8

Not sure why @theme-ui/mdx@0.3.4 wants @mdx-js/react@1.6.21. I added

"resolutions": {
    "@mdx-js/react": "^1.6.22",
    "@mdx-js/util": "^2.0.0-next.8",
    "remark-mdx": "^2.0.0-next.8"
  }

and that did not fix the issue (after a Gatsby clean).

I am completely lost here.

@hasparus
Copy link
Member

hasparus commented Dec 8, 2020

Thanks for the issue, @454de6e!

This is certainly something we should work on, but in the meantime could you try the following?

"resolutions": {
    "@mdx-js/mdx": "1.6.17",
    "@mdx-js/react": "1.6.17"
}

@maiertech
Copy link
Author

@hasparus Adding above resolutions does not fix the issue. Do I need to pin the version in the dependencies as well? And can you give me an idea of what you suspect is happening? Maybe that would help me trouble shoot further.

And I have been running into this Theme UI problem already some time ago, see here: UNStats/gatsby-themes#375. In this instance the issue was introduced when I tried bumping @mdx-js/react from ^1.5.3 to ^1.5.7. The solution was to freeze to 1.5.5. Since then the issue kept reappearing and disappearing as I upgraded Gatsby related dependencies.

What ever happens, it basically breaks the core of theme-ui. Though I am not sure what is different in my example repo that is different from other repos where it works.

@atanasster
Copy link
Collaborator

@454de6e - I tried your example and a couple things come to attention

grab121

  • Here is a colored anchor once I added a color: 'red' styling for styles.a

grab122

@atanasster
Copy link
Collaborator

Oh, and I have it frozen at 2.0.0-next.7 if that makes a difference

"resolutions": {
    "@mdx-js/react": "2.0.0-next.7"
  }

in https://github.com/maiertech/gatsby-themes/blob/theme-ui-styles-broken/package.json

@maiertech
Copy link
Author

maiertech commented Dec 9, 2020

Thank you @atanasster for looking into this! You are absolutely right, I goofed up styles.a in a recent refactoring in which I lost the color prop. I updated @maiertech/preset to fix this and applied it. I also fixed the other styling issues that you noticed. With these changes in place, I still saw the un-styled links resulting from MDX transformation.

Just to clarify, Theme UI styling works fine outside the MDX transformation. When I use Theme UI components, including Styled styles are applied as expected.

To fix the MDX transformation I settled on

"resolutions": {
  "@mdx-js/react": "^1.6.22"
}

in line with what you suggested. This fixes it, but I still don't fully understand why. I see now in the devtools that the inner MDXProvider has the correct components prop (merging with components from the outer MDXProvider), which was not the case before applying the fix. This probably hints at a context issue, but what exactly is happening and how can theme-ui address this?

To understand this would be important, because the underlying Gatsby themes will only work correctly when the resolutions fix is applied to the package.json of the consuming Gatsby site, which is not ideal.

@atanasster
Copy link
Collaborator

@454de6e - great to hear its fixed now for you. You are correct, its an issue with context versions.

I will close this, but please open a new issue if you have any further troubles with theme-ui :)

@hasparus
Copy link
Member

hasparus commented Dec 9, 2020

in line with what you suggested. This fixes it, but I still don't fully understand why. I see now in the devtools that the inner MDXProvider has the correct components prop (merging with components from the outer MDXProvider), which was not the case before applying the fix. This probably hints at a context issue, but what exactly is happening and how can theme-ui address this?

To understand this would be important, because the underlying Gatsby themes will only work correctly when the resolutions fix is applied to the package.json of the consuming Gatsby site, which is not ideal.

@454de6e React Contexts seem to be compared "by reference". Multiple versions of a package exposing context provider in your deps result in context provider not connecting to context consumer.

@atanasster @lachlanjc Should we change our MDX dep to a peer dep?

@atanasster
Copy link
Collaborator

@hasparus - I havent researched in-depth but this seems a one-off issue with mdxjs (they had pinned versions of some packages causing multiple versions to be loaded) and afaics they have "fixed" it as of june this year: mdx-js/mdx#865

For a peer dep, I am not really supportive at this point - however we should upgrade the mdxjs/react dependencies to ^1.6.22:

"@mdx-js/react": "^1.0.0",

@maiertech
Copy link
Author

@atanasster @hasparus @lachlanjc After giving this some more thought, I think bumping above dependency will not fix this for good. The root cause of the issue was me using gatsby-plugin-mdx together with theme-ui. @mdx-js/react is a peer dependency for gatsby-plugin-mdx but at the same time it is hard-wired in @theme-ui/mdx. Sooner than later versions will diverge and then we would end up with conflicting contexts again.

When using theme-ui in an MDX project (which of course is optional because theme-ui can be used without any MDX), I would expect you would already bring your own @mdx-js/react as peer dependency of whatever framework you use, e.g. gatsby-plugin-mdx. In that case the hard-wired dependency is in the way.

But I also see your point that you want theme-ui usable without the hassle of having to install peer dependencies. So I wonder if maybe Theme UI should also have an official way of using it modularly, where rather than installing theme-ui you reach for specific packages like @theme-ui/components or @theme-ui/mdx (complemented by @theme-ui/core) depending on your specific styling needs. Then @theme-ui/mdx can have @mdx-js/react as peer dependency and whoever wants the complete experience can install theme-ui which comes with @mdx-js/react as dependency.

@atanasster
Copy link
Collaborator

atanasster commented Dec 10, 2020

@454de6e thanks a lot for the feedback. You raise some very valid points, but just to clarify - the issue is with earlier versions of mdxjs/react that have pinned their react version (no jiggle room):

"react": "16.13.1",
"react-dom": "16.13.1",

Here is a branch from earlier this year:
https://github.com/mdx-js/mdx/blob/5b511baa475c59b7baf449c268e14669862a5cb9/package.json#L99.

This is no longer the case with the latest mdxjs/react` 1.6.22, where react is a peerDependency:
https://github.com/mdx-js/mdx/blob/510bae2580958598ae29047bf755b1a2ea26cf7e/packages/react/package.json#L35

To cut it short - I think if we install a newer version of mdxjs/react, the issue will not happen.

However, your point of installing mdxjs/react only as part of theme-ui, while its kept a peerDependency in @theme-ui/mdx is valid.
The downside is - well, why should we install mdxjs/react if a user is not ever going to use mdx, and potential breaking backward compatibility.
In any case - I do think your suggestion has merits worth making the change, but lets see what the other team members think.

Regardless, thank you very much for taking the time to think about this and come up with a very viable suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants