-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add a remote-dom to remote-ui adapter #511
base: main
Are you sure you want to change the base?
Conversation
c8e1a10
to
364cac0
Compare
8283f6e
to
d57efb4
Compare
/snapit |
🫰✨ Thanks @robin-drexler! Your snapshots have been published to npm. Test the snapshots by updating your "@remote-dom/core": "0.0.0-snapshot-20241204183932",
"@remote-dom/polyfill": "0.0.0-snapshot-20241204183932",
"@remote-dom/preact": "0.0.0-snapshot-20241204183932",
"@remote-dom/react": "0.0.0-snapshot-20241204183932",
"@remote-dom/signals": "0.0.0-snapshot-20241204183932" |
/snapit |
🫰✨ Thanks @igor10k! Your snapshots have been published to npm. Test the snapshots by updating your "@remote-dom/core": "0.0.0-snapshot-20241204204334",
"@remote-dom/polyfill": "0.0.0-snapshot-20241204204334",
"@remote-dom/preact": "0.0.0-snapshot-20241204204334",
"@remote-dom/react": "0.0.0-snapshot-20241204204334",
"@remote-dom/signals": "0.0.0-snapshot-20241204204334" |
/snapit |
🫰✨ Thanks @igor10k! Your snapshots have been published to npm. Test the snapshots by updating your "@remote-dom/core": "0.0.0-snapshot-20241204205017",
"@remote-dom/polyfill": "0.0.0-snapshot-20241204205017",
"@remote-dom/preact": "0.0.0-snapshot-20241204205017",
"@remote-dom/react": "0.0.0-snapshot-20241204205017",
"@remote-dom/signals": "0.0.0-snapshot-20241204205017" |
/snapit |
🫰✨ Thanks @robin-drexler! Your snapshots have been published to npm. Test the snapshots by updating your "@remote-dom/core": "0.0.0-snapshot-20241204224457",
"@remote-dom/polyfill": "0.0.0-snapshot-20241204224457",
"@remote-dom/preact": "0.0.0-snapshot-20241204224457",
"@remote-dom/react": "0.0.0-snapshot-20241204224457",
"@remote-dom/signals": "0.0.0-snapshot-20241204224457" |
9659e48
to
e9d04ef
Compare
584ccad
to
fdd1a75
Compare
fdd1a75
to
590eb6d
Compare
/snapit |
🫰✨ Thanks @igor10k! Your snapshots have been published to npm. Test the snapshots by updating your "@remote-dom/core": "0.0.0-snapshot-20250107180300",
"@remote-dom/polyfill": "0.0.0-snapshot-20250107180300",
"@remote-dom/preact": "0.0.0-snapshot-20250107180300",
"@remote-dom/react": "0.0.0-snapshot-20250107180300",
"@remote-dom/signals": "0.0.0-snapshot-20250107180300" |
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.
Had a bunch of requests, but this is looking really great 👍
packages/core/source/legacy/host.ts
Outdated
const index = tree[id]?.findIndex(({slot}) => slot === key) ?? -1; | ||
|
||
if (index !== -1) { | ||
records.push([ |
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.
Should we check if the new fragment is actually different from the old one, before removing it?
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.
Can it be the same? I assume that there's some shallow comparison happening on the remote to not cause redundant payloads. I mean if the fragment didn't change then there should be no "update props" action.
/snapit |
9336ffd
to
53c1b2e
Compare
/snapit |
🫰✨ Thanks @igor10k! Your snapshots have been published to npm. Test the snapshots by updating your "@remote-dom/core": "0.0.0-snapshot-20250110191257",
"@remote-dom/polyfill": "0.0.0-snapshot-20250110191257",
"@remote-dom/preact": "0.0.0-snapshot-20250110191257",
"@remote-dom/react": "0.0.0-snapshot-20250110191257",
"@remote-dom/signals": "0.0.0-snapshot-20250110191257" |
Co-authored-by: Chris Sauve <[email protected]>
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 it would be nicer to introduce this as a separate package
packages/core/package.json
Outdated
@@ -81,6 +91,7 @@ | |||
}, | |||
"dependencies": { | |||
"@remote-dom/polyfill": "workspace:^1.4.2", | |||
"@remote-ui/core": "^2.0.0", |
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.
re: https://github.com/Shopify/remote-dom/pull/511/files#r1909454081
It also struck me as "not right" for @remote-dom/core
to depend on @remote-ui/core
.
What about, instead, extracting all the adapter parts to a new package like @remote-dom/compat
? Is it possible to do that? This would allow core
to remain focused on remote-dom
exclusively while still allowing someone to use @remote-dom/compat
if they need remote-ui
backwards compatibility.
/snapit |
🫰✨ Thanks @oluwatimio! Your snapshots have been published to npm. Test the snapshots by updating your "@remote-dom/core": "0.0.0-snapshot-20250115141927",
"@remote-dom/polyfill": "0.0.0-snapshot-20250115141927",
"@remote-dom/preact": "0.0.0-snapshot-20250115141927",
"@remote-dom/react": "0.0.0-snapshot-20250115141927",
"@remote-dom/signals": "0.0.0-snapshot-20250115141927" |
e753b82
to
c409934
Compare
/snapit |
🫰✨ Thanks @igor10k! Your snapshot has been published to npm. Test the snapshot by updating your "@remote-dom/compat": "0.0.0-snapshot-20250116203606" |
Hey folks, just trying to figure out if something is blocking this PR from being shipped? |
@simontaisne another review from @lemonmade would be good, I think. |
Close https://github.com/Shopify/checkout-web/issues/40150.
The adapter is supposed to help transition from
remote-ui
toremote-dom
, while supporting existing legacy code that usesremote-ui
-style APIs.