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

frehner · 2022-04-08T18:01:02Z

Description

Part of #66, and depends on #1054 being merged.

The package split! A better version of #628, this time with:

Package exports
Typescript types
A solution for Jest tests to resolve correctly (requires a change in the dev's project, but should work)
No eslint complaints
The page refreshes on changes to hydrogen-ui client and server components

Quick followup work that's planned:

Shopify/hydrogen#66 (comment)

Additional context

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

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 according to your change
Run yarn changeset add if this PR cause a version bump based on Keep a Changelog and adheres to Semantic Versioning

github-actions · 2022-04-08T18:01:20Z

We detected some changes in packages/*/package.json or packages/*/src, and there are no updates in the .changeset.
If the changes are user-facing and should cause a version bump, run "yarn changeset add" to track your changes and include them in the next release CHANGELOG.
If you are making simple updates to examples or documentation, you do not need to add a changeset.

frehner · 2022-04-08T18:01:30Z

examples/template-hydrogen-default/src/App.server.jsx


 function App({routes}) {
  return (
    <Suspense fallback={<LoadingFallback />}>
      <ShopifyProvider shopifyConfig={shopifyConfig}>
        <CartProvider>
          <DefaultSeo />
+          <TestServer />
+          <TestClient />
+          {COOLNUMBER}


I'll remove this before merging, this is just if anyone wants to pull it down and test it locally

frehner · 2022-04-08T20:25:08Z

packages/hydrogen/src/framework/plugin.ts

+        path.join(
+          // eslint-disable-next-line node/no-missing-require
+          path.dirname(require.resolve('@shopify/hydrogen-ui/client'))
+        ),


@frandiox @jplhomer This causes the CI build failures (because the e2e test package hasn't installed hydrogen-ui. But this will also be true of devs already building on hydrogen, too).

So I think we have a couple of options here:

We leave this as is, but that means we also require that everyone install the hydrogen-ui. This probably isn't ideal, as it would mean that as soon as this PR is merged then it's a breaking change (and we don't have hydrogen-ui publishing to npm yet either, so devs can't even install the stub)

We modify the Vite plugin to take in additional "componentPaths" and we spread them here. Which means for the demo store we can add hydrogen-ui

We wait on Improve component discoverability in client build #994

Thoughts? Other options?

I think it fails because resolve.require cannot find the package, right? In that case, can we just wrap it in try/catch and only pass it if the package is actually found? 🤔

Yeah, I actually tried doing that last week and for some reason it wasn't working. I'll try it again and give some better details as to why

Worked this time. I must have been doing something weird last week 🤷 👍

frehner · 2022-04-08T20:34:08Z

examples/template-hydrogen-default/src/App.server.jsx

@@ -6,13 +6,21 @@ import DefaultSeo from './components/DefaultSeo.server';
 import NotFound from './components/NotFound.server';
 import LoadingFallback from './components/LoadingFallback';
 import CartProvider from './components/CartProvider.client';
+import {Test as TestServer} from '@shopify/hydrogen-ui/server';
+import {
+  // Test as TestClient,


@frandiox @jplhomer It maybe appears that tree-shaking isn't working here; when I run a build for the template-hydrogen-default package, I can do a search in the dist/ folder for the contents of TestClient (which is the word mycoolcomponentclient) and find instances of that in every build, both server and client.

As far as I can tell, I've implemented the necessary "magic" which should allow hydrogen-ui components to be tree-shaken. I could be wrong, but I think things are correct there.

So, the problem is either:

hydrogen-ui isn't configured correctly for tree-shaking

The template-hydrogen-default build script isn't correctly tree-shaking

Our Vite plugin doesn't properly allow tree-shaking to happen

Something else?

What do you think?

This is something I've seen in Vite before. Everything in index.ts is not tree-shaken for some reason. It needs further research but I'm hoping we can indicate some kind of "moduleSideEffect: false" for every component and let the tree-shake work. Otherwise we'll need to import from different paths 🙈

It's strange that Vite doesn't pick up on sideEffects: false in package.json for this, since I believe that it normally does

Opened #1071 to continue this discussion

frehner · 2022-04-08T21:34:30Z

packages/hydrogen-ui/package.json

+    }
+  },
+  "typesVersions": {
+    "<=4.6": {


Package exports supported in 4.7 https://devblogs.microsoft.com/typescript/announcing-typescript-4-7-beta/#package-json-exports-imports-and-self-referencing

If package.exports becomes a problem with TS or Jest, we can also simply rely on real files at the root like we do in hydrogen (client.js and server.js next to dist directory).

They shouldn't be, I've gotten TS (all versions) and Jest all figured out with package exports! 👍 Check out the CONTRIBUTING.md file for details.

cartogram · 2022-04-12T17:34:02Z

packages/hydrogen-ui/CONTRIBUTING.md

@@ -0,0 +1,11 @@
+## Package exports notes:
+
+- Until [Jest can correctly resolve package.exports](https://github.com/facebook/jest/issues/9771), here's a workaround:


Curious if you know, would vitest would also require a workaround?

Not sure right off hand, but I can look into it!

Though I am planning on converting tests over to Vitest for our own internal components tests.

* v1.x-2022-07: (37 commits) Adds Shopify/hydrogen-cli for development server (#1089) Make `useUrl()` reactive for subscribing to navigation events (#1082) fix e2e tests streaming and suspense draft (#1033) [Hydrogen docs]: Restructure and simplify IA (#1079) Use new storefront API headers in queryShop API helper (#1075) Remove the dep of hydrogen-ui for now until we get it publishing (#1076) [ci] release v1.x-2022-07 (#1048) Introduce a synchronous-looking version of fetch for server and client (#983) improve the current type of ShopifyContextValue to be based on ShopifyConfig (#1072) Foundational work to split hydrogen-ui to its own package (#1060) Allow returning new requests from API routes (#1057) [Hydrogen docs]: Analytics (#962) Analytics (#890) Allow line breaks inside script tags (#1061) Fix encoding of CSS Modules to solve hydration errors (#1062) [Hydrogen docs]: Update guidance on troubleshooting third-party dependencies (#1055) Update Vite RSC Plugin (#1054) Fix link prefetch (#1059) Initial devTools component (#998) ...

Foundational work to split hydrogen-ui to its own package

b4ed450

frehner commented Apr 8, 2022

View reviewed changes

frehner marked this pull request as draft April 8, 2022 18:12

frehner mentioned this pull request Apr 8, 2022

update all components' exports to be consistent #1058

Closed

4 tasks

frehner commented Apr 8, 2022

View reviewed changes

Showcase example of no tree-shaking

cae13fb

frehner commented Apr 8, 2022

View reviewed changes

Support package.exports for TS 4.7

873d9ae

frehner force-pushed the split-packages branch from 0c4bbf2 to 873d9ae Compare April 8, 2022 21:35

blittle force-pushed the v1.x-2022-07 branch 3 times, most recently from 2976ae8 to 13376ef Compare April 11, 2022 15:27

try/catch require for h-ui

0f1c15f

blittle closed this Apr 11, 2022

blittle reopened this Apr 11, 2022

Merge branch 'v1.x-2022-07' into split-packages

e4878f2

frehner mentioned this pull request Apr 12, 2022

Treeshaking is not working #1071

Open

Remove example code for merging

8b3f14d

frehner requested a review from a team April 12, 2022 16:05

frehner marked this pull request as ready for review April 12, 2022 16:05

cartogram reviewed Apr 12, 2022

View reviewed changes

cartogram approved these changes Apr 12, 2022

View reviewed changes

frehner merged commit f10a512 into v1.x-2022-07 Apr 12, 2022

frehner deleted the split-packages branch April 12, 2022 20:35

frehner mentioned this pull request Apr 12, 2022

Split hydrogen-ui and hydrogen #66

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

frehner commented Apr 8, 2022 •

edited

Loading

github-actions bot commented Apr 8, 2022

frehner Apr 8, 2022

frehner Apr 8, 2022 •

edited

Loading

frandiox Apr 11, 2022

frehner Apr 11, 2022

frehner Apr 11, 2022

frehner Apr 8, 2022 •

edited

Loading

frandiox Apr 11, 2022

frehner Apr 11, 2022 •

edited

Loading

frehner Apr 12, 2022

frehner Apr 8, 2022

frandiox Apr 11, 2022

frehner Apr 11, 2022 •

edited

Loading

cartogram Apr 12, 2022

frehner Apr 12, 2022

frehner Apr 12, 2022

		@@ -0,0 +1,11 @@
		## Package exports notes:

		- Until [Jest can correctly resolve package.exports](https://github.com/facebook/jest/issues/9771), here's a workaround:

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

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

Conversation

frehner commented Apr 8, 2022 • edited Loading

Description

Additional context

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

github-actions bot commented Apr 8, 2022

Choose a reason for hiding this comment

frehner Apr 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

frehner Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frehner Apr 11, 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

frehner Apr 11, 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

frehner commented Apr 8, 2022 •

edited

Loading

frehner Apr 8, 2022 •

edited

Loading

frehner Apr 8, 2022 •

edited

Loading

frehner Apr 11, 2022 •

edited

Loading

frehner Apr 11, 2022 •

edited

Loading