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

breaking(gatsby-plugin-emotion): update to emotion@11 #27981

Merged
merged 20 commits into from
Nov 23, 2020
Merged

breaking(gatsby-plugin-emotion): update to emotion@11 #27981

merged 20 commits into from
Nov 23, 2020

Conversation

theskillwithin
Copy link
Contributor

@theskillwithin theskillwithin commented Nov 12, 2020

Upgrade gatsby-plugin-emotion package to use the new @emotion/react dep.

with new emotion update, this package no longer works unless these deps are upgraded

Fixes #28063

@theskillwithin theskillwithin requested review from a team as code owners November 12, 2020 02:08
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Nov 12, 2020
@theskillwithin theskillwithin requested review from damianstasik and removed request for a team November 12, 2020 21:33
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

packages/gatsby-plugin-emotion/package.json Outdated Show resolved Hide resolved
@wardpeet wardpeet added status: awaiting author response Additional information has been requested from the author topic: frontend Relates to frontend issues (e.g. reach/router, gatsby-link, page-loading, asset-loading, navigation) and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Nov 13, 2020
@wardpeet wardpeet self-assigned this Nov 13, 2020
@wardpeet wardpeet changed the title gatsby-plugin-emotion Upgrade deps to use @emotion/react [critical] Nov 13, 2020
@pvdz pvdz added the breaking change If implemented, this proposed work would break functionality for older versions of Gatsby label Nov 13, 2020
@kimbaudi
Copy link
Contributor

looking forward to this update to emotion@11

@jHards jHards mentioned this pull request Nov 15, 2020
@theskillwithin
Copy link
Contributor Author

@wardpeet yes thats a good idea. thanks for your help.

it seems I have tests failing still =[

When reviewing this PR please also check:

https://emotion.sh/docs/emotion-11 - this has all the package renaming

&&

looking at babel-preset-css-prop https://emotion.sh/docs/@emotion/babel-preset-css-prop

A Babel preset to automatically enable Emotion’s css prop when using the classic JSX runtime. If you want to use the new JSX runtimes please do not use this preset but rather just include our @emotion/babel-plugin directly and follow instructions for configuring the new JSX runtimes here.

conclusion

It looks like there is more work that can be done here to improve this gatsby plugin. However I am afraid I do not quite understand whats going on in gatsby-node.js for this plugin. I might have bitten off more than I can chew here.

@kimbaudi
Copy link
Contributor

kimbaudi commented Nov 15, 2020

@theskillwithin - there seems to be a lot of places where you need to replace the following:

  • @emotion/core to @emotion/react
  • jest-emotion to @emotion/jest
  • babel-plugin-emotion to @emotion/babel-plugin

But I think the reason why the test is failing is because emotion v11 changed autoLabel type from a boolean with a default value of true to a string with a default value of dev-only.

So you need to update gatsby-node.js in packages/gatsby-plugin/emotion/src folder.

from:

exports.pluginOptionsSchema = ({ Joi }) =>
  Joi.object({
    ...
    autoLabel: Joi.boolean()
      .default(process.env.NODE_ENV !== `production`)
      .description(
        `This option automatically adds the label property to styles so that class names generated by css or styled include the name of the variable the result is assigned to. You can read more about this option in babel-plugin-emotion's docs: https://emotion.sh/docs/babel-plugin-emotion#autolabel`
      ),
    ...
  })

to:

exports.pluginOptionsSchema = ({ Joi }) =>
  Joi.object({
    ...
    autoLabel: Joi.string()
      .default(`dev-only`)
      .description(
        `This option automatically adds the label property to styles so that class names generated by css or styled include the name of the variable the result is assigned to. You can read more about this option in @emotion/babel-plugin's docs: https://emotion.sh/docs/@emotion/babel-plugin#autolabel`
      ),
    ...
  })

I created a PR on your fork of gatsby repo with the remaining changes (https://github.com/theskillwithin/gatsby/pull/1).
Can you review and apply changes to this commit?

in emotion v11:
- @emotion/core → @emotion/react
- emotion-theming → moved into @emotion/react
- babel-plugin-emotion → @emotion/babel-plugin
- jest-emotion → @emotion/jest

update gatsby-plugin-emotion's autoLabel option from boolean to string
to reflect breaking changes in emotion v11.
wardpeet
wardpeet previously approved these changes Nov 16, 2020
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Awesome! This looks great, I'll test it out!

Thank you and congrats on your first contribution! 🎉

@LekoArts
Copy link
Contributor

@ward It seems like the changes in https://github.com/theskillwithin/gatsby/pull/1 should also be incorporated

update gatsby-plugin-emotion, gatsby docs, and packages references to emotion v11
@jHards
Copy link
Contributor

jHards commented Nov 16, 2020

@ward It seems like the changes in theskillwithin#1 should also be incorporated

now merged but checks failing

@kimbaudi
Copy link
Contributor

@jHards - sorry, the ci/circleci: bootstrap is failing because I edited packages/gatsby-admin/src/components/recipes-gui/resource-message.js. I'll fix it.

@kimbaudi
Copy link
Contributor

all checks have passed!

@kimbaudi
Copy link
Contributor

kimbaudi commented Nov 17, 2020

the build failed during integration testing. Not sure what is the cause since the code hasn't changed since the last build. I tried to resolve merge conflict, but the build failed. So I reverted my changes, but the build is still failing...

 FAIL  __tests__/develop.js (10.24s)
  ● gatsby develop › starts a gatsby site on port 8000

    Command failed with exit code 1: /tmp/gatsby-cli-QNNnyx/node_modules/.bin/gatsby develop
    internal/modules/cjs/loader.js:582
        throw err;
        ^

    Error: Cannot find module '/home/circleci/project/integration-tests/gatsby-cli/gatsby-sites/gatsby-develop/.cache/tmp-1699-D13C9Oh9Bssz'

      internal/modules/cjs/loader.js:582
          throw err;
          ^
      Error: Cannot find module '/home/circleci/project/integration-tests/gatsby-cli/gatsby-sites/gatsby-develop/.cache/tmp-1699-FaeQMKUq5j9K'
      at makeError (integration-tests/gatsby-cli/node_modules/execa/lib/error.js:59:11)
      at handlePromise (integration-tests/gatsby-cli/node_modules/execa/index.js:114:26)
@GAO8A
Copy link

GAO8A commented Nov 20, 2020

This appears to be blocking the completion of Tutorial part 4 (and also 5) on the Gatsby website. Is there a temporary work around?

@theskillwithin
Copy link
Contributor Author

whats going on here?

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Tested and works great! 🙏 Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change If implemented, this proposed work would break functionality for older versions of Gatsby status: awaiting author response Additional information has been requested from the author topic: frontend Relates to frontend issues (e.g. reach/router, gatsby-link, page-loading, asset-loading, navigation)
9 participants