Skip to content

Add a remote-dom to remote-ui adapter#511

Merged
igor10k merged 32 commits intomainfrom
rd/legacy-adapter
Jan 28, 2025
Merged

Add a remote-dom to remote-ui adapter#511
igor10k merged 32 commits intomainfrom
rd/legacy-adapter

Conversation

@robin-drexler
Copy link
Member

@robin-drexler robin-drexler commented Nov 25, 2024

Close https://github.com/Shopify/temp-project-mover-Archetypically-20250512095102/issues/58.

The adapter is supposed to help transition from remote-ui to remote-dom, while supporting existing legacy code that uses remote-ui-style APIs.

@robin-drexler robin-drexler force-pushed the rd/legacy-adapter branch 3 times, most recently from c8e1a10 to 364cac0 Compare November 25, 2024 20:18
@robin-drexler robin-drexler force-pushed the rd/legacy-adapter branch 3 times, most recently from 8283f6e to d57efb4 Compare December 4, 2024 16:29
@robin-drexler
Copy link
Member Author

/snapit

@shopify-github-actions-access
Copy link
Contributor

🫰✨ Thanks @robin-drexler! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

"@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"

@igor10k
Copy link
Contributor

igor10k commented Dec 4, 2024

/snapit

@shopify-github-actions-access
Copy link
Contributor

🫰✨ Thanks @igor10k! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

"@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"

@igor10k
Copy link
Contributor

igor10k commented Dec 4, 2024

/snapit

@shopify-github-actions-access
Copy link
Contributor

🫰✨ Thanks @igor10k! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

"@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"

@igor10k igor10k changed the base branch from main to create-legacy-adaptors December 4, 2024 22:26
@igor10k igor10k changed the base branch from create-legacy-adaptors to main December 4, 2024 22:27
@robin-drexler robin-drexler changed the title Rd/legacy adapter legacy adapter Dec 4, 2024
@robin-drexler
Copy link
Member Author

/snapit

@shopify-github-actions-access
Copy link
Contributor

🫰✨ Thanks @robin-drexler! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

"@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"

@igor10k igor10k force-pushed the rd/legacy-adapter branch 2 times, most recently from 584ccad to fdd1a75 Compare January 7, 2025 17:47
@igor10k igor10k marked this pull request as ready for review January 7, 2025 17:49
@igor10k igor10k force-pushed the rd/legacy-adapter branch from fdd1a75 to 590eb6d Compare January 7, 2025 17:58
@igor10k igor10k requested a review from lemonmade January 7, 2025 18:01
@igor10k igor10k changed the title legacy adapter Add a remote-dom to remote-ui adapter Jan 7, 2025
@igor10k
Copy link
Contributor

igor10k commented Jan 7, 2025

/snapit

@shopify-github-actions-access
Copy link
Contributor

🫰✨ Thanks @igor10k! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

"@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"

@igor10k igor10k requested a review from kumar303 January 7, 2025 19:08
Copy link
Member

@lemonmade lemonmade left a 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 👍

const index = tree[id]?.findIndex(({slot}) => slot === key) ?? -1;

if (index !== -1) {
records.push([
Copy link
Member

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?

Copy link
Contributor

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.

@igor10k
Copy link
Contributor

igor10k commented Jan 10, 2025

/snapit

@igor10k
Copy link
Contributor

igor10k commented Jan 10, 2025

/snapit

@shopify-github-actions-access
Copy link
Contributor

🫰✨ Thanks @igor10k! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

"@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"

igor10k and others added 2 commits January 13, 2025 10:38
Copy link
Contributor

@kumar303 kumar303 left a 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

},
"dependencies": {
"@remote-dom/polyfill": "workspace:^1.4.2",
"@remote-ui/core": "^2.0.0",
Copy link
Contributor

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.

@oluwatimio
Copy link

/snapit

@shopify-github-actions-access
Copy link
Contributor

🫰✨ Thanks @oluwatimio! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

"@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"

@igor10k
Copy link
Contributor

igor10k commented Jan 16, 2025

/snapit

@shopify-github-actions-access
Copy link
Contributor

🫰✨ Thanks @igor10k! Your snapshot has been published to npm.

Test the snapshot by updating your package.json with the newly published version:

"@remote-dom/compat": "0.0.0-snapshot-20250116203606"

@simontaisne
Copy link
Member

Hey folks, just trying to figure out if something is blocking this PR from being shipped?

@robin-drexler
Copy link
Member Author

@simontaisne another review from @lemonmade would be good, I think.

Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

I'm lacking some context on remote-dom internals but it looks good to me. I played with the kitchen sink example and added some fragments to it in various places. Everything was working well ✨

Copy link
Member

@lemonmade lemonmade left a comment

Choose a reason for hiding this comment

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

Wonderful job — thank you all for maintaining the high bar for documentation and examples in this repo, and for elevating the bar on unit tests!

"access": "public",
"@remote-dom/registry": "https://registry.npmjs.org"
},
"version": "1.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Will the changeset being a major not put this to 2.0.0 on deploy? I assumed you'd want to start on a 0.x version instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with how versioning works in this repo. My goal was to have a 1.0.0 release for the compat package. Should I put 0.1 here as the version and let changesets bump it to 1.0.0?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think you should do that. IIRC, as soon as you merge to main it will deploy the 0.1.0 version, and then changesets will create an additional PR that will bump it to 1.0.0 and manage the changelogs.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for all these helpful, easy-to-read tests 💜

@igor10k igor10k merged commit 1a42bf6 into main Jan 28, 2025
6 checks passed
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.

6 participants