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

jsxImportSource pragma + jest + typescript #2069

Closed
Emiliano-Bucci opened this issue Nov 2, 2020 · 13 comments
Closed

jsxImportSource pragma + jest + typescript #2069

Emiliano-Bucci opened this issue Nov 2, 2020 · 13 comments
Labels

Comments

@Emiliano-Bucci
Copy link

Emiliano-Bucci commented Nov 2, 2020

Hi guys! i've recently update next js + react + emotion to the latest versions. I've started to use the new jsxImportSource pragma instead of the old one, and everything works fine. The only issue is that using Jest, in my snapshots is get You have tried to stringify object returned from \css\` function. It isn't supposed to be used directly (e.g. as value of the \`className\` prop), but rather handed to emotion so it can handle it (e.g. as value of \`css\` prop).. Is there a fix for this?

Also i wanted to ask something related to the eslint-plugin-emotion. Currently we're able to add the /* @jsx jsx */ comment directly on save. Do you know if it would be also possible to add the new jsxImportSource on save?

Thanks a lot! :)

@Andarist
Copy link
Member

Andarist commented Nov 2, 2020

As to the jest issue - hard to tell what's going on if you don't share a runnable repro case. It seems to be related to an incorrect configuration of some tool, but given that multiple tools are involved in this it's hard to tell based on the provided information.

Also i wanted to ask something related to the eslint-plugin-emotion. Currently we're able to add the /* @jsx jsx */ comment directly on save. Do you know if it would be also possible to add the new jsxImportSource on save?

It would be - could u file an issue for that?

@Emiliano-Bucci
Copy link
Author

@Andarist Yes sure, sorry, i forgot to add the repo :D

This is basically a starter repo i normally use -> https://github.com/Emiliano-Bucci/nextjs-starter

About the issue, sure, i'll open :)

@Andarist
Copy link
Member

Andarist commented Nov 2, 2020

Huh, this was a quirky one. next/babel preset doesn't detect React version on its own but rather it receives information about it from the caller:
https://github.com/vercel/next.js/blob/f7f376f91e4ef8fe9ddcfca7e871cf8195d5894c/packages/next/build/babel/preset.ts#L68-L70

In next context the caller is usually a webpack-loader and it checks the React version to provide this hasJsxRuntime flag:
https://github.com/vercel/next.js/blob/197d46ddb9bebc4a74360e66f50b091edd811ae0/packages/next/build/webpack-config.ts#L232-L238

When you are using Jest the caller is babel-jest and it doesn't provide any flag like this - it doesn't know about its existence for obvious reasons: it's a generic package. Therefore the @babel/preset-react that is contained in next/babel doesn't get runtime: 'automatic' provided to it.

I would recommend just adding our preset to the mix but there is some problem with duplicated JSX transforms in such a case that I've been looking into as part of this issue. I haven't yet figured it out but it seems like a problem with Babel (or I don't understand something but usually duplicated plugins should be deduped).

If you want to keep using pragma this should work:

module.exports = (api) => {
  api.cache.using(() => process.env.NODE_ENV);

  return {
    presets: [
      "next/babel",
      process.env.NODE_ENV === "test" && [
        "@emotion/babel-preset-css-prop",
        { runtime: "automatic" },
      ],
    ].filter(Boolean),
  };
};

But as you can control over Babel config then probably the best would be a variation of this and the solution I've figured out for that other issue (here) and that would look smth like:

module.exports = (api) => {
  api.cache.using(() => process.env.NODE_ENV);

  if (process.env.NODE_ENV === "test") {
    return {
      presets: [
        "next/babel",
        process.env.NODE_ENV === "test" && [
          "@emotion/babel-preset-css-prop",
          { runtime: "automatic" },
        ],
      ],
    };
  }

  return {
    presets: [
      ["next/babel", { "preset-react": { importSource: "@emotion/core" } }],
    ],
    plugins: ["babel-plugin-emotion"],
  };
};

That test variant sort-of contains duplicated JSX transforms but it seems to work OK on the given repro.

@Emiliano-Bucci
Copy link
Author

@Andarist Thanks for the exhaustive explanation! I guess it took a while to figure out, so thanks for your time :) I've tried the last solution, but for some reason i'm still getting the same issue in the snapshots :/ In fact, the only reason i wasn't using the babel plugin over the pragma, it was that during the tests, with the snapshots i was getting that issue.

@Andarist
Copy link
Member

Andarist commented Nov 2, 2020

Could u try running ur tests with --no-cache flag? If it does then please clear your cache - you can do that by running tests with --showConfig and removing what gets printed under "cacheDirectory". If it doesn't - could u push out what u currently have on a branch? I will try to take a look tomorrow. With what I've proposed I have this output right now:

 FAIL  src/Components/index.test.tsx (5.167 s)
  test
    ✕ test (43 ms)

  ● test › test

    expect(received).toMatchSnapshot()

    Snapshot name: `test test 1`

    - Snapshot  - 1
    + Received  + 5

    + .emotion-0 {
    +   background: red;
    + }
    +
      <div>
        <div
    -     css="You have tried to stringify object returned from `css` function. It isn't supposed to be used directly (e.g. as value of the `className` prop), but rather handed to emotion so it can handle it (e.g. as value of `css` prop)."
    +     class="emotion-0"
        >
          Component
        </div>
      </div>

       7 |     const { container } = render(<Component />);
       8 | 
    >  9 |     expect(container).toMatchSnapshot();
         |                       ^
      10 |   });
      11 | });
      12 | 

      at Object.<anonymous> (src/Components/index.test.tsx:9:23)

 › 1 snapshot failed.

which is exactly what we have wanted to achieve.

@Emiliano-Bucci
Copy link
Author

@Andarist Great, it works now! (I run tests as you suggest with --no-cache). Thanks a lot! Now i can get rid of the pragma and use the babel plugin 🥇

@Andarist
Copy link
Member

Andarist commented Nov 3, 2020

I'm glad that it has worked out.

@Andarist Andarist closed this as completed Nov 3, 2020
@Andarist
Copy link
Member

Andarist commented Nov 4, 2020

The easiest solution (and currently the best one) would be to just do this:

module.exports = (api) => {
  api.cache(true);

  return {
    presets: [
      [
        "next/babel",
        {
          "preset-react": {
            runtime: "automatic",
            importSource: "@emotion/core",
          },
        },
      ],
    ],
    plugins: ["babel-plugin-emotion"],
  };
};

but this would require changes in the next/babel preset (which I'm going propose to them). In your case, as mentioned, the problem arises from the fact that jest doesn't provide the appropriate caller object to their preset right now. This configuration works OK if we only use webpack (and possibly some other tools provided by Next).

@Emiliano-Bucci
Copy link
Author

Emiliano-Bucci commented Nov 5, 2020

@Andarist But this would require to use the Pragma, right? So there's no possible to only use the@emotion/babel-preset-css-prop preset, right?

@Andarist
Copy link
Member

Andarist commented Nov 5, 2020

No - it wouldn't. Those options passed to @babel/preset-react allows this to work without pragma. There is just a problem with using this simple config right now with jest - I will be reporting this to the Next team today/tomorrow.

@Emiliano-Bucci
Copy link
Author

Emiliano-Bucci commented Nov 5, 2020

Oh ok; yes, in fact using this config also works without pragma

@Andarist
Copy link
Member

Andarist commented Nov 5, 2020

If you want you can check this PR with a lengthy explanation of the situation: vercel/next.js#18847

@Emiliano-Bucci
Copy link
Author

Thanks :)

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

No branches or pull requests

2 participants