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

feat(gatsby): Make __experimentalThemes part of plugin APIs #15144

Merged

Conversation

ChristopherBiscardi
Copy link
Contributor

@ChristopherBiscardi ChristopherBiscardi commented Jun 26, 2019

This change makes it so that what used to be called __experimentalThemes
is now part of the plugins APIs. This means that any plugin can take
advantage of the Theme APIs like Component Shadowing.

  • This change contains no functional changes to how themes work today. The way that is accomplished is by "patching" plugins to look like the theme logic wants it to before passing it in. After we deprecate and remove __experimentalThemes, we can change this logic to be more plugin-specific.
  • __experimentalThemes and plugins style themes can not be used at the
    same time. If __experimentalThemes is defined, it takes precedence
    so as to not break existing sites.

The plan here is to not break existing sites while enabling the stable
version of themes to be used. We (I most likely) should go back and remove
the __experimentalThemes handling after a suitable deprecation period.

I'd like to see this released under a tag so we can test it on a wider range of sites before merging

TODO

  • Add deprecation message for __experimentalThemes (should we do this in this PR or another?)
  • Should we expand the webpack handling of themes in packages/gatsby/src/utils/webpack.config.js to work on all plugins? What's the status of feat(gatsby): enable babel-loader for all dependencies #14111 in relation to this?

@ChristopherBiscardi
Copy link
Contributor Author

dupe fragments in the e2es

error GraphQL Error There was an error while compiling your site's GraphQL queries.
  Error: RelayParser: Encountered duplicate defintitions for one or more documents: each document must have a unique name. Duplicated documents:
- GatsbyImageSharpFixed
- GatsbyImageSharpFixed_tracedSVG
- GatsbyImageSharpFixed_withWebp
- GatsbyImageSharpFixed_withWebp_tracedSVG
- GatsbyImageSharpFixed_noBase64
- GatsbyImageSharpFixed_withWebp_noBase64
- GatsbyImageSharpFluid
- GatsbyImageSharpFluid_tracedSVG
- GatsbyImageSharpFluid_withWebp
- GatsbyImageSharpFluid_withWebp_tracedSVG
- GatsbyImageSharpFluid_noBase64
- GatsbyImageSharpFluid_withWebp_noBase64
- GatsbyImageSharpResolutions
- GatsbyImageSharpResolutions_tracedSVG
- GatsbyImageSharpResolutions_withWebp
- GatsbyImageSharpResolutions_withWebp_tracedSVG
- GatsbyImageSharpResolutions_noBase64
- GatsbyImageSharpResolutions_withWebp_noBase64
- GatsbyImageSharpSizes
- GatsbyImageSharpSizes_tracedSVG
- GatsbyImageSharpSizes_withWebp
- GatsbyImageSharpSizes_withWebp_tracedSVG
- GatsbyImageSharpSizes_noBase64
- GatsbyImageSharpSizes_withWebp_noBase64

@ChristopherBiscardi
Copy link
Contributor Author

e2e_tests_production_runtime has a weird require error. not sure if this is an e2e-related setup issue or an actual themes issue

error Cannot find module 'gatsby-plugin-global-style'


  Error: Cannot find module 'gatsby-plugin-global-style'
  
  - loader.js:581 Function.Module._resolveFilename
    internal/modules/cjs/loader.js:581:15
  
  - v8-compile-cache.js:162 Function.require.resolve
    [production-runtime]/[v8-compile-cache]/v8-compile-cache.js:162:23
  
  - index.js:27 
    [production-runtime]/[gatsby]/dist/bootstrap/load-themes/index.js:27:43
  
  - Generator.next
  
  - new Promise
  
  - index.js:45 resolveTheme
    [production-runtime]/[gatsby]/dist/bootstrap/load-themes/index.js:45:17
  
  - index.js:111 
    [production-runtime]/[gatsby]/dist/bootstrap/load-themes/index.js:111:32
  
  - Generator.next
  
  - new Promise

@ChristopherBiscardi
Copy link
Contributor Author

It looks like https://github.com/gatsbyjs/gatsby/pull/15179/files means we can remove the theme logic from the query compiler, which also fixes the duplicate fragments issue here

@@ -257,17 +257,7 @@ module.exports = async (program, directory, suppliedStage) => {
rules.media(),
rules.miscAssets(),
]
if (store.getState().themes.themes) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to keep this part of the plugin for now as I reverted full node_modules traversal from #15270.

I'll remove this piece in a follow up PR where I traverse through plugins again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to remove once #15284 is in

This change makes it so that what used to be called `__experimentalThemes`
is now part of the `plugins` APIs. This means that any plugin can take
advantage of the Theme APIs like Component Shadowing.

* This change contains no functional changes to how themes work today.
* `__experimentalThemes` and `plugins` style themes can not be used at the
  same time. If `__experimentalThemes` is defined, it takes precedence
  so as to not break existing sites.

The plan here is to not break existing sites while enabling the stable
version of themes to be used. We (I most likely) should go back and remove
the `__experimentalThemes` handling after a suitable deprecation period.

**I'd like to see this released under a tag so we can test it on a wider range of sites before merging**

\# TODO

- [ ] Add deprecation message for __experimentalThemes (should we do this in this PR or another?)
- [ ] Should we expand the webpack handling of themes in `packages/gatsby/src/utils/webpack.config.js` to work on all plugins? I think I remember another PR
Because of gatsbyjs#14111, we can remove the custom theme processing.
@sidharthachatterjee
Copy link
Contributor

#15179 fixes the duplicated fragment issue

@pieh
Copy link
Contributor

pieh commented Jul 2, 2019

Failing tests are result of duplicated plugin entries (fix in #15307 )

Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

@sidharthachatterjee sidharthachatterjee changed the title Make __experimentalThemes part of plugin APIs feat(gatsby): Make __experimentalThemes part of plugin APIs Jul 2, 2019
@sidharthachatterjee sidharthachatterjee merged commit 3dce8cb into gatsbyjs:master Jul 2, 2019
@sidharthachatterjee
Copy link
Contributor

Published in gatsby@2.13.0 and holy crap, we have themes in stable now 💃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants