Refactor [breaking change]: Separate hydrogen UI into own package #628

frehner · 2022-02-08T18:58:06Z

Description

Part of #66

Additional context

Currently I haven't actually pulled over any of the components yet; I have focused mainly on getting the build settings, lerna config, etc. all correct as much as possible. That said, I have tested importing the ui library into the example repo and it works, so there's that.

Before submitting the PR, please make sure you do the following:

Add your change under the Unreleased heading in the package's CHANGELOG.md
Read the Contributing Guidelines
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123)
Update docs in this repository for your change, if needed

…rogen into refactor-hydrogen-ui

frehner · 2022-02-08T18:58:23Z

lerna.json

@@ -1,9 +1,8 @@
 {
-  "lerna": "4.0.0",


This is no longer needed in v4

frehner · 2022-02-08T18:58:51Z

lerna.json

  "packages": [
    "packages/*"
  ],
-  "version": "0.10.1",
+  "version": "independent",


HUI is going to be versioned completely different than the rest, so this is necessary.

frehner · 2022-02-08T18:59:20Z

packages/hydrogen-ui/ComponentPlayground.tsx

@@ -0,0 +1,20 @@
+import {StrictMode} from 'react';


Mainly just a thing from Vite, we can remove this file if we want.

frehner · 2022-02-08T18:59:59Z

packages/hydrogen-ui/package.json

+  "name": "@shopify/hydrogen-ui",
+  "version": "2022.04.0-alpha",
+  "description": "Components for modern custom Shopify storefronts",
+  "homepage": "https://github.com/Shopify/hydrogen#readme",


Above this comment: placeholders

frehner · 2022-02-08T19:00:21Z

packages/hydrogen-ui/package.json

+  ],
+  "main": "./dist/hydrogen-ui.umd.min.js",
+  "module": "./dist/hydrogen-ui.es.min.js",
+  "exports": {


package exports!

Did you get this working with Jest and TypeScript 😮
We have this PR Shopify/hydrogen#263 that is blocked by those two. I've recently found some kind of unofficial resolver for Jest that works with package exports, I should try that.

For TS, in my very simple example the types were working correctly when using H-UI in the example repo. This is because I defined a types property in package.json, and TS will use that to look for typedefs. No need for TS to understand package exports in this case.

However, based on the conversation below about getting more complex with our package exports, I'll have to test this again once we have separate builds (assuming that's the route we go), to see if we can still get that to work or not.

Regarding Jest, I didn't test that. Would Jest necessarily need to understand package exports, or could people mock out the H-UI package to get around that? (And for tests within H-UI itself, all the file resolution would just be relative paths, right? ...I'm probably missing something, though)

About Jest, if you import any component from H-UI in your app, Jest won't know how to resolve it. So you can only test your app if you mock all H-UI components, which is a lot and maybe misses the point of testing 🤔
In any case, I think we could try https://github.com/k-g-a/jest-node-exports-resolver

frehner · 2022-02-08T19:01:20Z

packages/hydrogen-ui/package.json

+    }
+  },
+  "types": "types/index.d.ts",
+  "sideEffects": false,


VERY important to get tree shaking to work. As we add components, we'll have to go through them and make sure to add them to an array here, if they have side effects. (Note: things inside of useEffect() are fine, this is more about global side effects such as adding to window or something like that. e.g. Boomerang stuff?)

frehner · 2022-02-08T19:01:50Z

packages/hydrogen-ui/tsconfig.json

+    "resolveJsonModule": true,
+    "isolatedModules": true,
+    "declaration": true,
+    "emitDeclarationOnly": true,


Have TS only emit declarations; the rest of the work is done by Vite.

frehner · 2022-02-08T19:04:06Z

packages/hydrogen-ui/tsconfig.json

+    "isolatedModules": true,
+    "declaration": true,
+    "emitDeclarationOnly": true,
+    "declarationDir": "./types",


It's a little bit easier to put the compiled types into a types/ folder than into the dist/ folder, since Vite cleans the dist folder on build. I could change that up, and put it all into the dist/ folder, but this was easier in the moment. Not a big deal to change up if so desired.

frehner · 2022-02-08T19:07:05Z

examples/template-hydrogen-default/package.json

@@ -35,6 +35,7 @@
  "dependencies": {
    "@headlessui/react": "^1.4.1",
    "@shopify/hydrogen": "^0.10.1",
+    "@shopify/hydrogen-ui": "^2022.04.0-alpha",


I was just testing that this worked, and it does. Currently it doesn't do anything in the PR because I removed that code.

frehner · 2022-02-08T19:07:41Z

packages/hydrogen-ui/index.html

@@ -0,0 +1,12 @@
+<!DOCTYPE html>


In conjunction with the ComponentPlayground file above; can be removed.

frehner · 2022-02-08T19:07:59Z

packages/hydrogen-ui/package.json

+  "homepage": "https://github.com/Shopify/hydrogen#readme",
+  "license": "MIT",
+  "engines": {
+    "node": ">=14"


Probably move this to 16?

frehner · 2022-02-08T19:08:47Z

packages/hydrogen-ui/package.json

+  "module": "./dist/hydrogen-ui.es.min.js",
+  "exports": {
+    ".": {
+      "import": "./dist/hydrogen-ui.es.min.js",


Do we want to have "prod" and "dev" builds for this library? I can make that work if we want to, but for the time being I just left it as a prod build.

jplhomer

🚀 super great. I really appreciate the time you put into documenting this PR with comments 😍

benjaminsehl · 2022-02-09T01:17:10Z

+1 to what Josh said. Those comments are really considerate and helped me a lot. Thanks!

frandiox · 2022-02-09T05:11:39Z

@frehner Great, excited to see progress on this! And thanks so much for the comments!

Perhaps it's is just because this is an empty boilerplate but it seems this intends to bundle all components in hydrogen-ui.min.js?

That wouldn't work in RSC because it depends on file extensions (/\.client\.[jt]sx?$/). Therefore, we would need to bundle + transpile every component separately so we end up with dist/CartXYZ.client.js, dist/Seo.client.js, etc. (I think this could be done with Rollup's this.emitFile).

An alternative would be bundling in 1 file for other non-RSC frameworks but provide a different import path that gets the source files so we can use them in Hydrogen framework. Maybe we can even use an alias in the framework so it's transparent to users (@shopify/hydrogen-ui/src/* => @shopify/hydrogen-ui/*) 🤔

frehner · 2022-02-09T18:53:34Z

Perhaps it's is just because this is an empty boilerplate but it seems this intends to bundle all components in hydrogen-ui.min.js?

You're correct.

That wouldn't work in RSC because it depends on file extensions (/\.client\.[jt]sx?$/). Therefore, we would need to bundle + transpile every component separately so we end up with dist/CartXYZ.client.js, dist/Seo.client.js, etc. (I think this could be done with Rollup's this.emitFile).

An alternative would be bundling in 1 file for other non-RSC frameworks but provide a different import path that gets the source files so we can use them in Hydrogen framework. Maybe we can even use an alias in the framework so it's transparent to users (@shopify/hydrogen-ui/src/* => @shopify/hydrogen-ui/*) 🤔

This is interesting, and I honestly don't know if it's been figured out yet or not.

Does the community at large have an answer to this question? For example, are any other UI libraries bundling for RSC, and if so, what are they doing and how are they doing it?

When looking at the RFC, one specific section seems to be called out as a potential solution to this (and a more detailed section below that): a conditional packages exports called "react-server".

The example they have there:

"exports": {
    ".": {
      "react-server": {
        "browser": "./debugger-polyfill.server.js",
        "node": "./native-impl.server.js"
      },
      "default": {
        "browser": "./browser-impl.client.js",
        "node": "./node-polyfill.client.js"
      }
    }
  }

Here's my interpretation of that doc and the files, let me know if you think I've gone wrong anywhere:

We would make two different bundles: a hydrogen-ui.server.js and hydrogen-ui.client.js. I'll use their package exports example above to tie into what I think our equivalent would be:

react-server.browser's file debugger-polyfill.server.js seems to be an idea that you can render server components in a browser env for debugging, but I don't really know what that means / how it works.
react-server.node's file native-impl.server.js appears to be what we would call hydrogen-ui.server.js and contains all the H-UI server components
default.browser's file browser-impl.client.js seems to be what we would call hydrogen-ui.client.js and contains the client components
default.node's file node-polyfill.client.js seems to be a wrapper or helper for hydrogen-ui.client.js for SSR rendering

With all that said... does this seem like it would work? Is that the correct path forward? Would love your thoughts @frandiox @jplhomer (and anyone else)

Side note regarding this custom react-server condition: node itself can resolve these conditions using user conditions flag... but do bundlers know it? According to webpack, some do. But I don't know if that's good enough for us, or what effect that would have on devs.

jplhomer · 2022-02-09T18:58:47Z

Yeah, that makes sense @frehner. This is definitely an untrodden path, so it's on us to come up with some opinions that will help inform the React ecosystem at large.

A couple thoughts:

What implications does bundling all client components together in a single file have on the end-result of the bundle? Is the bundler e.g. Vite going to be able to tree-shake? I honestly don't know.
Another option is to generate aliases for every single client component in package.json#exports. This sounds tedious, but it could be automated. This is perhaps a way to export single files if it means a smaller client bundle for the end user.

blittle · 2022-02-09T19:04:00Z

Agreed. All of this is new ground. I don't think anyone substantial in the community is yet building for RSC. I think we should explore some of the ideas here and answer the questions. Then come up with community guidelines.

frehner · 2022-02-09T20:46:42Z

What implications does bundling all client components together in a single file have on the end-result of the bundle? Is the bundler e.g. Vite going to be able to tree-shake? I honestly don't know.

We should still be able to build out to multiple files; it's just not what Vite does by default (and what I have happening now). But by getting into the Rollup config through Vite, I'm pretty sure I can get that working fairly easily and this won't be a concern.

(To clarify: we'll still have a single entry point file for .server and .client, but each component will be separated out into their own files from there)

Another option is to generate aliases for every single client component in package.json#exports. This sounds tedious, but it could be automated. This is perhaps a way to export single files if it means a smaller client bundle for the end user.

I think if we need to explore this route, that we can use package export paths ( "default": "dist/esm/" ) which allows anything in the dist/esm folder to be imported, without needing to define each one.

If I'm understanding you correctly here.

frehner · 2022-02-10T00:16:56Z

This is my last update on this for a bit, since our plan has changed and this work will be shelved for a tiny bit.

I made some updates to this PR, to work with different package exports for server/client. My initial impression is that, instead of trying to be magical, we can perhaps be more specific and have our package exports be like import {} from '@shopify/hydrogen-ui/server (and same for /client, so that you have access to both. It seems like on the server side of things, we would want access to both instead of only getting one or the other based on the environment?

With this setup with have two different entry points, and two different outputs: index.server.js and index.client.js.

Anyway, with this latest commit the static values are correctly imported, but ESLint doesn't like it so we have to put eslint ignore comments there.

The components that I import, however, appear to potentially be breaking by our Vite plugin; I get this error:

Uncaught Error: Could not find client component index-LTE2MDc5NjA2MTg
    importClientComponent react-server-dom-vite.js:44

Anyway, not something I don't think we have to pay attention to at the moment, since this is being put on the backburner for a bit.

As noted above in another comment I had with Fran, with this updated setup the TS types don't work yet either; that relies on TS support for package exports. (It was working before with a single file build, but that's not valid for us here now)

Anyway, fun times. Gives me something to think about as I work on other things for a bit.

frandiox · 2022-02-10T09:20:34Z

For the record, our implementation of RSC + Vite is not relying on Node's conditions (--conditions react-server). The main reason is that this requires running multiple processes in development and production, or creating multiple bundles beforehand, etc. This doesn't fit very well Vite in development and Oxygen in production. Therefore, instead of that, we are leveraging Vite hooks to load client components differently as we need:

In the browser, we always load the normal source code for every client component.
In the server, we have access to the real source code but also to something called "module reference", which is some kind of meta information that RSC needs. When running RSC, we use module references. Later, when running SSR using the output of RSC, we use the real source code for every client component. This all happens within the same process thanks to Vite transformations.

That's why relying on package exports and Node conditions is not an option unless we change how our RSC+Vite implementation works. There's more information about all of this here.

Aside from that, we are using Vite's glob imports to find all the client components in the filesystem. We need to pass the path to hydrogen-ui to the RSC plugin here.

After analyzing the content of this path, Vite will create separate bundles for every file that matches **/*.client.[jt]sx? and make them available in the main client bundle by adding some kind of "import map". This map takes a component ID as input (e.g. index-LTE2MDc5NjA2MTg) and returns a promise to the component and all its dependencies.

The components that I import, however, appear to potentially be breaking by our Vite plugin; I get this error:

I think you are getting that error because even though the server has imported and generated component IDs, the client doesn't have a proper Vite import map. You'll need to provide *.client.js files and pass the directory path to the Vite plugin.

frehner · 2022-02-10T17:12:15Z

For the record, our implementation of RSC + Vite is not relying on Node's conditions (--conditions react-server). The main reason is that this requires running multiple processes in development and production, or creating multiple bundles beforehand, etc. This doesn't fit very well Vite in development and Oxygen in production.

Makes sense, I agree. I didn't really like that solution either when I started working on it on the package side of things.

Therefore, instead of that, we are leveraging Vite hooks to load client components differently as we need:

In the browser, we always load the normal source code for every client component.

In the server, we have access to the real source code but also to something called "module reference", which is some kind of meta information that RSC needs. When running RSC, we use module references. Later, when running SSR using the output of RSC, we use the real source code for every client component. This all happens within the same process thanks to Vite transformations.

To be clear, when you say "source code" are you referring to the actual, original source files? If so: is that a sustainable practice for when libraries start getting built specifically for RSC? In other words, the node ecosystem very rarely publishes the actual source code to npm, and instead publishes compiled code; would RSC libraries have to change that to where they publish their source code to npm?

That's why relying on package exports and Node conditions is not an option unless we change how our RSC+Vite implementation works. There's more information about all of this here.

Yeah, agreed again that package exports AND node conditions just doesn't seem right here.

To be clear, though, package exports themselves should still be ok, right? All they really do is just map a name to a file, so I'm not sure why we wouldn't want to support that.

Aside from that, we are using Vite's glob imports to find all the client components in the filesystem. We need to pass the path to hydrogen-ui to the RSC plugin here.

Let me see if I understand this correctly: every library that supports RSC would have to be manually pass their path to our plugin in order to work correctly? If so: is that sustainable in the long-term?

But I could be completely misunderstanding here.

I think you are getting that error because even though the server has imported and generated component IDs, the client doesn't have a proper Vite import map. You'll need to provide *.client.js files and pass the directory path to the Vite plugin.

Ok, so I do provide a .client file in that case (the package export for /client maps to index.client.js), so hypothetically if I could get the Vite plugin to find my library, then it would work?

Thanks Fran, I appreciate all your feedback!

frehner · 2022-03-21T16:15:51Z

I'm going to use this as a reference, but start from scratch just due to how different the repo is at this point.

frehner added 3 commits February 8, 2022 11:37

refactor: create separate hydrogen-ui package

748a6e5

refactor: create separate hydrogen-ui package

5d0cf1b

Merge branch 'refactor-hydrogen-ui' of https://github.com/Shopify/hyd…

c66dabe

…rogen into refactor-hydrogen-ui

frehner commented Feb 8, 2022

View reviewed changes

lerna.json

@@ -1,9 +1,8 @@

{

"lerna": "4.0.0",

Copy link

Contributor Author

frehner Feb 8, 2022

Choose a reason for hiding this comment

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

This is no longer needed in v4

frehner commented Feb 8, 2022

View reviewed changes

jplhomer approved these changes Feb 8, 2022

View reviewed changes

feat: work on bundling for hydrogen-ui, and test things out

6aff59d

jplhomer changed the base branch from main to v1.x-2022-07 March 8, 2022 21:37

frehner closed this Mar 21, 2022

frehner mentioned this pull request Apr 8, 2022

Foundational work to split hydrogen-ui to its own package #1060

Merged

9 tasks

frehner deleted the refactor-hydrogen-ui branch June 7, 2022 20:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor [breaking change]: Separate hydrogen UI into own package #628

Refactor [breaking change]: Separate hydrogen UI into own package #628

frehner commented Feb 8, 2022

frehner Feb 8, 2022

frehner Feb 8, 2022

frehner Feb 8, 2022 •

edited

Loading

frehner Feb 8, 2022

frehner Feb 8, 2022

frandiox Feb 9, 2022

frehner Feb 9, 2022

frandiox Feb 10, 2022

frehner Feb 8, 2022 •

edited

Loading

frehner Feb 8, 2022

frehner Feb 8, 2022

frehner Feb 8, 2022

frehner Feb 8, 2022

frehner Feb 8, 2022

frehner Feb 8, 2022

jplhomer left a comment

benjaminsehl commented Feb 9, 2022

frandiox commented Feb 9, 2022 •

edited

Loading

frehner commented Feb 9, 2022 •

edited

Loading

jplhomer commented Feb 9, 2022

blittle commented Feb 9, 2022

frehner commented Feb 9, 2022 •

edited

Loading

frehner commented Feb 10, 2022 •

edited

Loading

frandiox commented Feb 10, 2022

frehner commented Feb 10, 2022 •

edited

Loading

frehner commented Mar 21, 2022

Refactor [breaking change]: Separate hydrogen UI into own package #628

Refactor [breaking change]: Separate hydrogen UI into own package #628

Conversation

frehner commented Feb 8, 2022

Description

Additional context

Before submitting the PR, please make sure you do the following:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frehner Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frehner Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jplhomer left a comment

Choose a reason for hiding this comment

benjaminsehl commented Feb 9, 2022

frandiox commented Feb 9, 2022 • edited Loading

frehner commented Feb 9, 2022 • edited Loading

jplhomer commented Feb 9, 2022

blittle commented Feb 9, 2022

frehner commented Feb 9, 2022 • edited Loading

frehner commented Feb 10, 2022 • edited Loading

frandiox commented Feb 10, 2022

frehner commented Feb 10, 2022 • edited Loading

frehner commented Mar 21, 2022

frehner Feb 8, 2022 •

edited

Loading

frehner Feb 8, 2022 •

edited

Loading

frandiox commented Feb 9, 2022 •

edited

Loading

frehner commented Feb 9, 2022 •

edited

Loading

frehner commented Feb 9, 2022 •

edited

Loading

frehner commented Feb 10, 2022 •

edited

Loading

frehner commented Feb 10, 2022 •

edited

Loading