theme.styles are not applied in Gatsby site

maiertech commented 3 years ago

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

Clone https://github.com/maiertech/gatsby-themes/tree/theme-ui-styles-broken
Run yarn
Run yarn workspace digital-garden-example run dev
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.

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

Additional context This looks like https://github.com/system-ui/theme-ui/issues/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 commented 3 years ago

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 commented 3 years ago

@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: https://github.com/UNDataForum/gatsby-themes/issues/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 commented 3 years ago

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

I could not exactly reproduce the issue as you describe it above - I do not see any anchors on that page
I added some anchors and found out that links's colors are overridden in the posts list page (not sure if thats intentional)
1. https://github.com/maiertech/gatsby-themes/blob/f1b2f87cb84134306492c43881718f3ddf3d2d53/packages/gatsby-theme-digital-garden/src/%40maiertech/gatsby-theme-posts-core/components/posts-page.js#L38
2. https://github.com/maiertech/gatsby-themes/blob/f1b2f87cb84134306492c43881718f3ddf3d2d53/packages/gatsby-theme-digital-garden/src/%40maiertech/gatsby-theme-tags-core/components/tag-page.js#L47
The theme however does not have any color styling for styles.a, so it would be expected that the anchors are not showing color primary - here is a screenshot:

grab121

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

grab122

atanasster commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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: https://github.com/mdx-js/mdx/issues/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: https://github.com/system-ui/theme-ui/blob/8616794f86eb254b86321001ff4433720fcfb468/packages/mdx/package.json#L17

maiertech commented 3 years ago

@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 commented 3 years ago

@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.

system-ui / theme-ui

theme.styles are not applied in Gatsby site #1345