Skip to content

@guardian/ab-react | Use react jsx runtime instead of @emotion/react jsx runtime - #489

Merged
joecowton1 merged 2 commits into
mainfrom
mm/fix-ab-react-jsx
Mar 21, 2023
Merged

joecowton1 merged 2 commits into
mainfrom
mm/fix-ab-react-jsx

Conversation

@coldlink

@coldlink coldlink commented Mar 17, 2023

Copy link
Copy Markdown
Member

What are you changing?

Why?

In identity we noticed an interesting error, where @guardian/ab-react was doing some "interesting" emotion stuff when we imported it causing the bundle to break in the gateway project when attempting to update @guardian/ab-react to a version greater than 3.x.x.

emotion-cache.esm.js:281 Uncaught TypeError: _virtual_stylis_js__WEBPACK_IMPORTED_MODULE_0__.s.exports.middleware is not a function
    at createCache (emotion-cache.esm.js:281:26)
    at ./node_modules/@guardian/ab-react/esm/node_modules/.pnpm/@emotion_react@11.0.0_wk7fohhuxwcjfgq2kdoh4ny7by/node_modules/@emotion/react/dist/emotion-element-14b67335.esm.js (emotion-element-14b67335.esm.js:15:65)
    at __webpack_require__ (bootstrap:19:1)
    at ./node_modules/@guardian/ab-react/esm/node_modules/.pnpm/@emotion_react@11.0.0_wk7fohhuxwcjfgq2kdoh4ny7by/node_modules/@emotion/react/jsx-runtime/dist/emotion-react-jsx-runtime.esm.js (emotion-element-14b67335.esm.js:138:57)
    at __webpack_require__ (bootstrap:19:1)
    at ./node_modules/@guardian/ab-react/esm/libs/@guardian/ab-react/src/context.js (stylis.js:4:35)
    at __webpack_require__ (bootstrap:19:1)
    at ./src/client/static/hydration.tsx (ophan.ts:37:3)
    at __webpack_require__ (bootstrap:19:1)
    at ./src/client/static/index.tsx (hydration.tsx:68:3)

Screenshot 2023-03-17 at 10 30 27

I noticed that at the top of node_modules/@guardian/ab-react/esm/libs/@guardian/ab-react/src/context.js in Gateway was importing jsx from emotion instead:

import { jsx } from '../../../../node_modules/.pnpm/@emotion_react@11.0.0_wk7fohhuxwcjfgq2kdoh4ny7by/node_modules/@emotion/react/jsx-runtime/dist/emotion-react-jsx-runtime.esm.js';
...

I saw that the @guardian/ab-react has a dev dependency on @emotion/react. Looking into why this was added I noticed this commit: 02f8ac1 (#371)

While this allows the build to pass, it also adds a dependency on the emotion jsx runtime to the output file!

For @guardian/ab-react I think should be using the default react jsx runtime.

So after some trial and error, I updated the local tsconfig.json in @guardian/ab-react to do this.

With this make build compiles correctly, and I see in dist/libs/@guardian/ab-react/esm/context.js that the top of the file now uses, which should remove the dependency on @emotion/react!

import { jsx } from 'react/jsx-runtime';

@coldlink
coldlink requested a review from a team as a code owner March 17, 2023 12:28
@changeset-bot

changeset-bot Bot commented Mar 17, 2023

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 5e1b8fa

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@guardian/ab-react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions Bot added @guardian/ab-react 📦 npm Affects a @guardian package on NPM labels Mar 17, 2023
@coldlink

coldlink commented Mar 17, 2023

Copy link
Copy Markdown
Member Author

Okay that's interesting, it was building locally fine https://cloud.nx.app/runs/P4FM4RwGNr, but CI is failing 🤔

@coldlink

Copy link
Copy Markdown
Member Author

Interesting the build works when I build it by itself using

npx nx run @guardian/ab-react:build

https://nx.app/runs/lirOldvecT

@coldlink
coldlink force-pushed the mm/fix-ab-react-jsx branch from 542f03c to e81ab0b Compare March 17, 2023 13:00
@mxdvl

mxdvl commented Mar 17, 2023

Copy link
Copy Markdown
Contributor

Good luck with this one–I explored enforcing the order of builds in #373, which made CI pass, but was deemed utltimately a indadequate solution to this problem.

Discussing with @joecowton1 it seemed like as we require Emotion for all consumers of Source, it was acceptable to make it a peer dependency of this project.

For your information, dotcom-rendering no longer loads @guardian/ab-react in favour of using SWR like a provider & current implementation.

@sndrs

sndrs commented Mar 17, 2023

Copy link
Copy Markdown
Contributor

yeah we should 100% not depend on build order to stop typescript breaking!

we should probably not define defaults for JSX handlers at all #490

@coldlink

Copy link
Copy Markdown
Member Author

yeah we should 100% not depend on build order to stop typescript breaking!

we should probably not define defaults for JSX handlers at all #490

This seems like a better approach to me!

@sndrs
sndrs requested review from a team as code owners March 17, 2023 14:53
@coldlink
coldlink force-pushed the mm/fix-ab-react-jsx branch from c62b042 to 0cc0d0a Compare March 17, 2023 15:04
@coldlink
coldlink force-pushed the mm/fix-ab-react-jsx branch from 0cc0d0a to 5e1b8fa Compare March 17, 2023 15:06

@mxdvl mxdvl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Excellent!

I could not crack it in #371 so this is a much welcomed simplification.

@arelra arelra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice detective work!

@joecowton1

Copy link
Copy Markdown
Contributor

@coldlink should we merge this one?

@coldlink

Copy link
Copy Markdown
Member Author

@coldlink should we merge this one?

sure merge it when you like! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

@guardian/ab-react 📦 npm Affects a @guardian package on NPM

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants