-
Notifications
You must be signed in to change notification settings - Fork 211
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
feat(react): react wrapper generator #2745
Conversation
Branch Preview |
d1314ca
to
fdea201
Compare
Tachometer resultsCurrently, no packages are changed by this PR... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial thoughts.
It might be nice to have a couple of comments in the wrapper plugin and the icon generator, just for reference in the future.
@@ -23,7 +23,7 @@ const getChangedPackages = () => { | |||
let command; | |||
try { | |||
command = execSync( | |||
'yarn --silent lerna ls --since origin/main --json --loglevel silent' | |||
'yarn --silent lerna ls --since origin/main --json --loglevel silent --ignore "@swc-react/*"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this here for the short term. BUT, if we choose to make these packages build artifacts that aren't tracked, we might not need this 🤔
hmmm... I did a quick test. Looks like the typescript react project works fine - link. But the js project does not compile. By looking into the canary package, it seems like all the js files are missing. Here is a example from https://unpkg.com/browse/@swc-react/[email protected]/ |
@Westbrook Thanks for another canary release. Both the JS project and Typescript project work fine now. |
Code comment added. |
Looking at the TS project, I'm looking at how to get this to work nicely: <ActionGroup
onChange={(event) =>
alert((event.target as ReturnType<typeof ActionGroup>).selected[0])
}
selects="single"
selected={["third"]}
>
<ActionButton value="first">First</ActionButton>
<ActionButton value="second">Second</ActionButton>
<ActionButton value="third">Third</ActionButton>
</ActionGroup> It's very unhappy about the typing here: onChange={(event) =>
alert((event.target as ReturnType<typeof ActionGroup>).selected[0])
} Is there a version of this that makes better sense to you? I've not got a lot of React experience, and the best I could Google either comes back in vanilla JS or with a custom React component to handle the changes. React Spectrum seems to surface all the internal changes as callback specifically prepared for hooks, which seems a bridge to far at our current level of support. Does this point at a need to reexport the element class so that we can do something like the following? onChange={(event) =>
alert((event.target as ActionGroupElement).selected[0])
} |
@Westbrook, Code example extracted from the above link: import type {EventName} from '@lit-labs/react';
import * as React from 'react';
import {createComponent} from '@lit-labs/react';
import {MyElement} from './my-element.js';
export const MyElementComponent = createComponent({
tagName: 'my-element',
elementClass: MyElement,
react: React,
events: {
onClick: 'pointerdown' as EventName<PointerEvent>, // <<<<<
onChange: 'input',
},
}); User code: <MyElementComponent
onClick={(e: PointerEvent) => {
console.log('DOM PointerEvent called!');
}}
onChange={(e: Event) => {
console.log(e);
}}
/> |
@jianliao that's not all the way there, however. That tells us the type of the event, which is likely useful, but is easy to force the type of with TS on the listener side. Where the issue arises is being able to type the |
Hi @Westbrook,
import * as React from 'react';
import { createComponent } from '@lit-labs/react';
import type { EventName } from '@lit-labs/react';
import { ActionGroup as SpActionGroup } from '@spectrum-web-components/action-group';
import '@spectrum-web-components/action-group/sp-action-group.js';
export const ActionGroup = createComponent({
displayName: 'ActionGroup',
elementClass: SpActionGroup,
react: React,
tagName: 'sp-action-group',
events: {
longpress: 'longpress' as EventName<CustomEvent>,
change: 'change' as EventName<Event>,
},
});
export type ActionGroupType = EventTarget & SpActionGroup; // <<<< reexport type for event.target type casting
import "./App.css";
import { Theme } from "@swc-react/theme";
import { ActionGroup, ActionGroupType } from "@swc-react/action-group"; // <<<< import ActionGroupType for type casting
import { ActionButton, ActionButtonType } from "@swc-react/action-button"; // <<<< import ActionButtonType for type casting
import '@spectrum-web-components/theme/theme-light.js';
import '@spectrum-web-components/theme/scale-medium.js';
function App() {
const handleChange = (e: Event) => {
console.log((e.target as ActionGroupType).selected); // <<<< cast as ActionGroupType
};
const handleClick = (e: React.MouseEvent<ActionButtonType>) => { // <<<< React SyntheticEvent could also use this reexport type, too.
console.log(e.currentTarget.selected);
}
return (
<Theme theme="spectrum" scale="medium" color="light">
<div className="App">
<ActionGroup selects="single" selected={["third"]} change={handleChange}>
<ActionButton value="first">First</ActionButton>
<ActionButton value="second">Second</ActionButton>
<ActionButton value="third">Third</ActionButton>
</ActionGroup>
<ActionButton toggles onClick={handleClick}>Toggle</ActionButton>
</div>
</Theme>
);
}
export default App; Let me know if it sounds good to you. Reference: |
220bdbd
to
c59ca07
Compare
@Westbrook, |
c59ca07
to
371c606
Compare
New Canary Versions:
|
3153b8a
to
342d768
Compare
Table seems to be publishing its React components incorrectly: https://unpkg.com/browse/@swc-react/[email protected]/index.dev.js |
Hi @Westbrook, looks like the above Table wrapper issue was caused by the |
8c4d8d9
to
9897d0a
Compare
9897d0a
to
b69c810
Compare
b69c810
to
cda4912
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the last notes and then we can merge this through. Sound good?
"${iconType}" | ||
], | ||
"dependencies": { | ||
"@lit-labs/react": "1.1.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jianliao getting to the end here and taking a quick last look.
Do you see any issue with opening this dependency to ^1.1.1
so that consumers can take newer versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realizing that this is true in the non-icon version. To ensure we keep these two approaches aligned, does it make sense to abstract a couple pieces of these two wrapper files to a shared set of utilities?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved all the code logic to cem file to improve the code reuse and code gen template reuse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for leading this exploration @jianliao! I think we're good to bring this code into the library with the HUGE caveat that it the releases it relates to will continue to be very "beta" and very "low guarantee" until some future time when more resources can be committed to actively testing/maintaining/expanding this work. However, we're very happy to bring you in as code owner on these additions until a time like that comes about.
I'll be looking to make an non-canary release of these packages this week, I'll be sure to @ you in the announcement so you don't miss it.
Description
This is the PR to implement #2390. The generated react-wrapper could make SWC work with React@17 and React@18.
All file changes in react folder are generated by the script. You can test it after a regular full build with script
build:react
Related issue(s)
Motivation and context
@lit-labs/react provides a utility wrapper that makes a React component wrapper for a custom element class. The wrapper correctly passes React props to properties accepted by the custom element and listens for events dispatched by the custom element.
How has this been tested?
A smoking test is still under construction. But I already built a web app (spectrum-preview.corp.adobe.com, require VPN) using this wrapper, it works pretty well.
Screenshots (if appropriate)
All of the above widgets are spectrum-web-components with the react wrapper which uses the Spectrum express theme.
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main
.