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

Added new JSX entry points for the automatic runtime #1970

Merged
merged 28 commits into from
Oct 31, 2020

Conversation

FLGMwt
Copy link
Contributor

@FLGMwt FLGMwt commented Aug 12, 2020

What: Draft / PoC implementation of the automatic jsx runtime #1969

Why: Examine feasibility of supporting the automatic jsx runtime

How:

  1. I added handling for runtime === 'automatic in the babel css preset to configure for the automatic runtime
  2. I implemented jsx-runtime in @emotion/core mirroring the existing jsx function. (this was copied wholesale for ease of modification, it should be factored once the solution is finalized)
  3. I added a jsx-runtime preconstruct entrypoint configuration to @emotion/core to allow babel to import @emotion/core/jsx-runtime (need to figure out UMD)
  4. I added a new from-scratch webpack / babel / react playground app to test babel configuration

Needs discussion / solutioning

We need react@^17.0.0 for forwarding our modified jsx args to react/jsx-runtime's jsx. How do we conditionally depend on react^17 without forcing a client upgrade? can we?

I couldn't get UMDs to work with @emotion/core/jsx-transform. It errored looking for an external for react/jsx-runtime I expected this to be fixed by adding { globals: { 'react/jsx-runtime': 'JSXRuntime' } } based on this, but it didn't work. I just disabled the UMD build to get things working.

The JSX implementation is incomplete and needs factoring. I was able to get it working with almost the only changes being React.createElement => React's jsx, but I know there are issues with keys. I think I'll need some guidance here on how withEmotionCache works.

Checklist:

  • Documentation
  • Tests
  • Code complete
  • Changeset

@changeset-bot
Copy link

changeset-bot bot commented Aug 12, 2020

🦋 Changeset detected

Latest commit: 8f6271e

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

This PR includes changesets to release 2 packages
Name Type
@emotion/core Minor
@emotion/babel-preset-css-prop Minor

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

package.json Outdated Show resolved Hide resolved
packages/babel-preset-css-prop/src/index.js Outdated Show resolved Hide resolved
packages/babel-preset-css-prop/src/index.js Outdated Show resolved Hide resolved
"module": "dist/core.esm.js",
"browser": {
"./dist/core.cjs.js": "./dist/core.browser.cjs.js",
"./dist/core.esm.js": "./dist/core.browser.esm.js"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's weird that these are named core, yeah? I tried to make it jsx-runtime but preconstruct overrode it. In any case, it works. We have:

@emotion/core/dist/core.esm.js
@emotion/core/jsx-runtime/dist/core.esm.js

and so on. The imports seem to work in the minimal-react so 🤷‍♀️ maybe it doesn't matter.

Copy link
Member

Choose a reason for hiding this comment

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

yes, it doesnt matter - i agree the naming convention used for those files is confusing, we have already agreed in the past on making that change but I'm not sure when it will land

packages/core/package.json Show resolved Hide resolved
packages/core/src/jsx-runtime.js Outdated Show resolved Hide resolved
packages/core/src/jsx-runtime.js Outdated Show resolved Hide resolved
packages/core/src/jsx-runtime.js Outdated Show resolved Hide resolved
playgrounds/minimal-react/package.json Outdated Show resolved Hide resolved
playgrounds/minimal-react/src/App.js Outdated Show resolved Hide resolved
@Andarist
Copy link
Member

Please note that I have seen your PR - I just haven't yet time to look at it properly. We are also aware of this new feature - but it has not been released yet to React. We are also watching closely how TypeScript will handle this - there are still some unknowns for us.

@morlay
Copy link
Contributor

morlay commented Oct 12, 2020

could we just added a hack jsx-runtime until react/jsx-runtime support old react version (now seems only 17, if changes a lot, may broken emotion for old version).

const _jsx = require("@emotion/core").jsx;

exports.Fragment = require("react").Fragment;
exports.jsx = (Component, { children, ...otherProps }, maybeKey) => _jsx(Component, maybeKey ? { ...otherProps, key: maybeKey }: otherProps, children);
exports.jsxs = (Component, { children, ...otherProps }, maybeKey) => _jsx(Component, maybeKey ? { ...otherProps, key: maybeKey }: otherProps, ...children);

@Andarist
Copy link
Member

Yes, we probably could add smth like this to bring automatic runtime for Emotion users stuck with React 16 but it seems to be counter-productive. I would much more prefer to push the ecosystem forward and use the actual new factories available in the React 17.

@SimenB
Copy link
Contributor

SimenB commented Oct 14, 2020

users stuck with React 16

React just published backports of the new transform to v0.14, v15 and v16

https://github.com/facebook/react/releases/tag/0.14.10
https://github.com/facebook/react/releases/tag/v15.7.0
https://github.com/facebook/react/releases/tag/v16.14.0

@Andarist
Copy link
Member

Wow, this is unexpected. In such a case I guess we should aim at shipping this for Emotion 10 as well.

@FLGMwt are you still eager to work on this PR? or should I take over? I would appreciate any help so you working on it and me giving feedback, reviews etc is definitely what I would prefer. Please let me know.

@morlay
Copy link
Contributor

morlay commented Oct 15, 2020

@Andarist

if the fallback logic still needed. facebook/react#20031

Have to keep old jsx of @emotion/core, and alias to createElement.

@Andarist
Copy link
Member

Andarist commented Oct 18, 2020

I've pushed some minor commits, cleaned up repo-related stuff etc. Gonna continue work on this later today/tomorrow.

TODO:

  • review & try to dedupe the classic/automatic runtimes in our code
  • tests
  • add dev runtime

Comment on lines +126 to +127
"react/jsx-runtime": "ReactJSX",
"react/jsx-dev-runtime": "ReactJSXDev"
Copy link
Member

Choose a reason for hiding this comment

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

These don't actually exist but we still want UMD builds for Emotion so these are just to make it so Preconstruct can make a build

@@ -0,0 +1,35 @@
// @flow
import * as ReactJSXRuntimeDev from 'react/jsx-dev-runtime'
Copy link
Contributor

Choose a reason for hiding this comment

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

OOC do you have a particular reason to prefer the namespace import over named ones? Does it work around webpack's (0, r.jsx)(...) compile output, or something?

Copy link
Member

Choose a reason for hiding this comment

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

This mostly should have no effect on the generated code later. It doesn't matter if you use the namespace import or individual imports until you reexport things or pass them as arguments to some functions.

We need to export things using the same names as defined in those runtimes so it's easier to use a namespace import here rather than renaming them or naming our exports differently and only exporting them under the specified names.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI while exploring this, I discovered that the namespace import is actually preferable for the reason I suspected (it bundles more efficiently), but only if the runtime is CJS. So it makes no difference for Emotion but it does make a difference for React. I opened facebook/react#20113 to suggest changing the React Babel plugin output to use a namespace import. 🙂

@billyjanitsch
Copy link
Contributor

@Andarist you mentioned wanting some help with docs in #2052 (comment). I'm happy to help; can you answer a few questions?

  • Since this hasn't been merged, would you like me to submit a PR to @FLGMwt's fork?
  • Is there anything specific that you'd like documented other than the new runtime option?
  • If there's anything high-level, can you give me a general pointer to the area of the docs where you'd like it mentioned?

@Andarist
Copy link
Member

Since this hasn't been merged, would you like me to submit a PR to @FLGMwt's fork?

I don't think it matters much but actually, it's probably best to prepare a separate PR against master because we auto-deploy docs from the master and there will be a gap between merging this PR and doing a final release having those changes (even if just a few hours or whatever). So it would be best to merge a docs update after we release this to avoid potential confusion among users looking at the docs that got updated too quickly 😉

Is there anything specific that you'd like documented other than the new runtime option?

We should add:

  • specific docs for things affected directly here - so probably just the docs for the preset.
  • explain different options on how one can integrate Emotion nowadays in their app, mentioning both new runtimes, older @jsx etc, each should explain briefly when it should be used (mainly env/tool requirements)

At all places where we are going to mention new runtimes, we should mention that they require the latest React 16 or React 17 and the latest version of @emotion/core.

If there's anything high-level, can you give me a general pointer to the area of the docs where you'd like it mentioned?

The most detailed "guide" explaining the old and new approaches should probably be mentioned here: https://emotion.sh/docs/css-prop

@billyjanitsch
Copy link
Contributor

billyjanitsch commented Oct 21, 2020

Thanks, this is really useful. I'll try to get a PR up tonight.

Update: unfortunately I got wrapped up in some other work tonight. I'll try to spend some workday time on it tomorrow (since we use it at work and are excited to upgrade!)

@billyjanitsch
Copy link
Contributor

Sorry @Andarist, the docs are taking me longer than expected, mainly because I only use the CSS prop Babel transform and hadn't initially considered the pragma option. The product of those two options and the two runtime options all need to be documented, and it's tricky to explain four possible options with slightly different constraints/outcomes in a way that isn't confusing. I'll try to have a PR up soon.

@cybercitizen7
Copy link

Hey guys,

Was this fixed? I just created a new React App and installed emotions library. When I try to use it like this:

/** @jsx jsx */
import {jsx} from '@emotion/core'

But I still get the error message:
pragma and pragmaFrag cannot be set when runtime is automatic.

Here is my package.json dependencies:

  "dependencies": {
    "@testing-library/jest-dom": "^5.11.9",
    "@testing-library/react": "^11.2.5",
    "@testing-library/user-event": "^12.6.3",
    "react": "^17.0.1",
    "react-dom": "^17.0.1",
    "react-scripts": "4.0.2",
    "web-vitals": "^1.1.0"
  },

I haven't done any other changes, just a pure newly created React App with command create-react-app

@karlhorky
Copy link

You're probably looking for:

/** @jsxImportSource @emotion/react */

As in the docs:

If you are using a zero-config tool with automatic detection of which runtime (classic vs. automatic) should be used and you are already using a React version that has the new JSX runtimes (hence runtime: 'automatic' being configured automatically for you) such as Create React App 4 then /** @jsx jsx / pragma might not work and you should use /* @jsxImportSource @emotion/react */ instead.

Source: https://emotion.sh/docs/css-prop#jsx-pragma:~:text=Create%20React%20App%204%20then%20%2F**,use%20%2F**%20%40jsxImportSource%20%40emotion%2Freact%20*%2F%20instead.

geektalent pushed a commit to geektalent/bookshelf that referenced this pull request Oct 25, 2022
goldapple911 added a commit to goldapple911/bookshelf that referenced this pull request Aug 15, 2023
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

9 participants