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

cloneElement support #1729

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

cloneElement support #1729

wants to merge 2 commits into from

Conversation

danieldelcore
Copy link
Collaborator

What is this change?

Compiled now treats react.cloneElement as a stylable element.

this can be extended to support react.createElement fairly easily.

Why are we making this change?

To allow cases where a CSS prop is used like so:

import { cloneElement } from 'react';
import { css } from '@compiled/react';

const MyDiv = ({ children }) => cloneElement(children, { css: css({ fontSize: 12 }) });

How are we making this change?

  • Adds logic to the babel plugin to make it aware of cloneElement CallExpression
  • Looks at the second argument (props) for an inline css prop
  • (this is fragile when indirection is introduced, open to feedback on how to address this properly)
  • Rename the object key to className
  • Wrap call in compiled runtime API

This might not be a direction the library wants to go in, which is understandable.
Alternatively, we could address this via <ClassNames> however Compiled currently does not pick up non-jsx elements as children of the <ClassNames> API.


PR checklist

Don't delete me!

I have...

  • Updated or added applicable tests
  • Updated the documentation in website/

Copy link

changeset-bot bot commented Oct 22, 2024

⚠️ No Changeset found

Latest commit: b6b9736

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

Copy link

netlify bot commented Oct 22, 2024

Deploy Preview for compiled-css-in-js canceled.

Name Link
🔨 Latest commit b6b9736
🔍 Latest deploy log https://app.netlify.com/sites/compiled-css-in-js/deploys/6719e27085f8e30008cecac9

},
});

// // Second pass to replace all usages of `style`.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TODO, makes sense to hook the style prop up as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does anything else ever use style in Babel? style={css(…)} isn't valid, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the pass over the style element is intended for outputting CSS vars 🤔 or something of that nature.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's only when it's in runtime mode, but yeah, maybe right…really glad if you can learn this part of it because I have no clue how extraction or runtime actually work 🤣

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Neither hahah

Copy link
Collaborator

@kylorhall-atlassian kylorhall-atlassian left a comment

Choose a reason for hiding this comment

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

Without pulling it up locally, I'm not of much help reviewing the AST, but great like spike into Compiled, I hope it's not too far off and they'll like—it will drastically reduce some technical debt as the alternative is using <ClassNames>{({ css }) => cloneElement(…, { className: ax([css({…}), props.className]) })}</ClassNames> or something.

import { css } from '@compiled/react';

const MyDiv = ({ children }) => {
return cloneElement(children, { css: css({ fontSize: 12 }) });
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is would be the full example of what we need this for—do you think it would work?

const styles = css({ fontSize: 12 });
const MyDiv = ({ children, className }) => {
  return cloneElement(children, { css: cx(styles, props.className) });
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

The other question I've got, probably for Compiled, is should this be props.css or props.className exclusively?

Copy link
Collaborator Author

@danieldelcore danieldelcore Oct 23, 2024

Choose a reason for hiding this comment

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

I had the same question.

I'll add a test case for the example you provided! It should just work because that part of the algorithm gets handed back to compiled

},
});

// // Second pass to replace all usages of `style`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does anything else ever use style in Babel? style={css(…)} isn't valid, right?

// TODO: This is a temporary fix to prevent the plugin from crashing when the second argument of cloneElement is not an object expression.
if (!t.isObjectExpression(props)) {
throw new Error('Second argument of cloneElement must be an object expression.');
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If someone is using a variable/spread/etc it'll probably be impossible to reliably fish out and modify the CSS prop.
Are there any standard approaches to this problem in use in the repo already? Do we throw/Log an err/try really hard to locate the variable?

@@ -295,6 +296,19 @@ export default declare<State>((api) => {
path: NodePath<t.TaggedTemplateExpression> | NodePath<t.CallExpression>,
state: State
) {
if (
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this check that cloneElement is imported from react too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes definitely and also should check for aliased imports

import { cloneElement as foo } from 'react';

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Edit: I added support for alias and member expressions in the latest commit

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.

3 participants