Prepare for bundling #2087

isaacroldan · 2024-05-07T08:56:13Z

WHY are these changes introduced?

We plan to bundle the hydrogen package into shopify/cli. To do that we need to solve some peerDependencies imports.

WHAT is this pull request doing?

Import peerDependencies like Vite or Remix from the hydrogen project folder, even if the CLI is installed globally. We use require.resolve to find them.
Update cli-kit and oclif to latests versions
Change some assets folders so that they are generated in a way that works for the bundled output.

HOW to test your changes?

Post-merge steps

Checklist

I've read the Contributing Guidelines
I've considered possible cross-platform impacts (Mac, Linux, Windows)
I've added a changeset if this PR contains user-facing or noteworthy changes
I've added tests to cover my changes
I've added or updated the documentation

shopify · 2024-05-07T09:16:25Z

Oxygen deployed a preview of your prepare-for-bundling-2 branch. Details:

Storefront	Status	Preview link	Deployment details	Last update (UTC)
third-party-queries-caching	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 23, 2024 4:07 AM
Skeleton (skeleton.hydrogen.shop)	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 23, 2024 4:07 AM
optimistic-cart-ui	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 23, 2024 4:07 AM
vite	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 23, 2024 4:07 AM
subscriptions	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 23, 2024 4:07 AM
custom-cart-method	✅ Successful (Logs)	Preview deployment	Inspect deployment	May 23, 2024 4:07 AM

Learn more about Hydrogen's GitHub integration.

frandiox

🎉 It seems to be working well with the import/resolve trick, both local and global versions.

I've pushed a few commits to update package-lock and sync some other dependencies. Also left a few comments.

frandiox · 2024-05-08T01:21:23Z

packages/cli/src/commands/hydrogen/build.ts

+  const vitePath = require.resolve('vite', {paths: [root]});
+  const viteNodePath = joinPath(vitePath, '..', 'dist', 'node', 'index.js');


It makes me a bit nervous relying on package internal paths to resolve ESM.
Would it work if we just strip the last part of the path until ..../node_modules/vite so that import(...) resolves according to their own package.json? Or maybe that doesn't work?

Eventually we should we start requiring Node 20.6, which already supports import.meta.resolve(...).

Alternatively, we could rely on --experimental-import-meta-resolve flag for lower versions of Node (this returns a promise). We can restart the process to add this flag automatically like we do for --experimental-vm-modules: https://github.com/Shopify/hydrogen/blob/main/packages/cli/src/hooks/init.ts#L5

There's also a ponyfill but I haven't tried it: https://github.com/wooorm/import-meta-resolve

So the reason I had to do this is that vite has this export config:

"exports": { ".": { "import": { "types": "./dist/node/index.d.ts", "default": "./dist/node/index.js" }, "require": { "types": "./index.d.cts", "default": "./index.cjs" }

When you use import it correctly uses the node one. When you use require it imports the cjs version.

If you try to import('{path}/node_modules/vite') it doesn't work:

│ Directory import '/Users/isaac/Projects/hydronext/node_modules/vite' is not │ │ supported resolving ES modules imported from /Users/isaac/src/github.com/Sho │ │ pify/hydrogen/packages/cli/dist/commands/hydrogen/build.js │ │ Did you mean to import │ │ /Users/isaac/Projects/hydronext/node_modules/vite/dist/node/index.js? │

How would this import-meta-resolve help us here?

Tried with

await import.meta.resolve?.('vite', root)

And it resolves it in the current directory and not in root 🤔

Another alternative could be this:

const vitePath = require.resolve('vite', {paths: [root]}); const vitePackageJson = await findUpAndReadPackageJson(vitePath) as {content: {[key: string]: any}, path: string}; const viteNodeIndexFile = vitePackageJson.content.exports?.['.'].import.default const viteNodePath = joinPath(dirname(vitePackageJson.path), viteNodeIndexFile)

Load the package json and find the correct import there.

We could extract this so it's not repeated in many places and can be called like importViteFrom(root) or findVitePathFrom(root)

And it resolves it in the current directory and not in root 🤔

So I just learned something: the second parameter is ignored in the Node 20.6 (backproted to 18.19 actually) implementation to be spec compliant.
If you want the second param to work, you need to run with NODE_OPTIONS="--experimental-import-meta-resolve" h2 dev, which is an experimental feature. In this case we can't use it unless we add this flag automatically as mentioned in my previous comment. Perhaps not worth it for now.

Load the package json and find the correct import there.

I think this one would be safer, yes. The exports field can also be changed (['.'].import.default: string vs ['.'].import: string vs ['.'].default: string) but I guess we can check the common paths for ESM?

I think this one would be safer, yes. The exports field can also be changed (['.'].import.default: string vs ['.'].import: string vs ['.'].default: string) but I guess we can check the common paths for ESM?

Should we go with this approach? I guess we could have some kind of utility importLocal('...', {esm: true, from: '...'}) or similar that checks exports if available in the package, or fallback to module, then main etc.

yeah, i'll try to do that :)

I've updated all the imports, let me know what you think

frandiox · 2024-05-08T02:19:33Z

packages/cli/src/index.ts

+  'hydrogen:setup:vite': SetupVite,
+};
+
+export default COMMANDS;


What's the benefit of listing commands manually? Perf?

The CLI needs to explicitly import all commands from bundled dependencies, once bundled hydrogen won't act as a "plugin" anymore. The easiest way to do this is to export a list of all commands.

frandiox · 2024-05-08T02:26:48Z

packages/cli/tsup.config.ts

-        `${outDir}/${GENERATOR_TEMPLATES_DIR}/${GENERATOR_STARTER_DIR}`,
+        `${outDir}/lib/${GENERATOR_TEMPLATES_DIR}/${GENERATOR_STARTER_DIR}`,


Any reason to move the generated folder from dist/generator-templates to dist/lib/generator-templates? Doesn't it work at the root in global CLI?

nop, in the bundled CLI everything is at the root so we can't go look for anything at ../ (which is what we were doing for the generator templates here).

In order to look for them at ./, I had to put them inside lib, that way the import will work both in development and bundled.

Ah I see.

There's also a generated dist/virtual-routes folder created at build time, which is referenced in src/lib/virtual-routes.ts also with ... Do we need to change that one too?

I missed that one, but probably yes, where is that imported? what command do I need to run to test if it works?

This is only used for the classic remix compiler. Try npm run dev in examples/classic-remix. Then the routes /subrequest-profiler and /graphiql should be added dynamically from that folder.

The code is in lib/classic-compiler/dev.ts and lib/virtual-routes.ts. The actual virtual-routes folder is copied from packages/hydrogen at build time in packages/cli/tsup.config.ts.

packages/cli/package.json

Co-authored-by: Fran Dios <frankdiox@gmail.com>

frandiox · 2024-05-22T08:33:23Z

packages/cli/src/commands/hydrogen/setup.test.ts

+import {copy as copyWithFilter, createSymlink} from 'fs-extra/esm';
 import {
  inTemporaryDirectory,
-  copyFile,


Unrelated but it would be nice to make filter available in cli-kit's copyFile.

frandiox

Looking good! Let's try to test this in a new experimental release asap.

* Explicitly export CLI commands * Commit dist folder * Move peerDependencies to dependencies * Improve prettier imports * Some cleanup * Fix prettier import * Import vite with a dynamic path * Fix versions * Fix package.json * install remix * dynamic import for codegen * dynamic import for hydrogen-codegen * dynamic import for mini-oxygen * Build hydrogen * Dynamic import for graphql-config * Dynamic import for remix-run * Remove dist * some cleanup * Update package-lock.json * Update package-lock.json * Apply prettier format * Update some functions * Fix tests * Format * Fix more dynamic imports * Fix more dynamic imports * Some cleanup * format * Upgrade oclif and cli-kit * Update cli version in templates * Update minimum oxygen-cli version to bump oclif/core dep * Dedup package-lock.json * Update packages/cli/package.json Co-authored-by: Fran Dios <frankdiox@gmail.com> * Create import-utils file to extract all custom imports * Extract all local imports to common file * Format code * Update package-lock.json * Remove unnecessary overrides * Clean unused imports * Copy virtual routes inside lib * Fix CLI tests * Fix package-lock * Minor refactor * Changesets --------- Co-authored-by: Fran Dios <fran.dios@shopify.com> Co-authored-by: Fran Dios <frankdiox@gmail.com>

isaacroldan added 21 commits April 26, 2024 10:35

Explicitly export CLI commands

03262b4

Commit dist folder

402d681

Move peerDependencies to dependencies

e379f4e

Improve prettier imports

ac00e3f

Some cleanup

1694ca0

Fix prettier import

ea60d53

Import vite with a dynamic path

70cc371

Fix versions

4d3ee75

Fix package.json

1cdb89c

install remix

01b2c8f

dynamic import for codegen

b5b75c9

dynamic import for hydrogen-codegen

e37909c

dynamic import for mini-oxygen

fce25ad

Build hydrogen

6fd6942

Dynamic import for graphql-config

f78872c

Dynamic import for remix-run

fe8b892

Merge branch 'main' into prepare-for-bundling-2

9210981

Remove dist

88dcbc0

some cleanup

99e3341

Update package-lock.json

1aae2f4

Update package-lock.json

635ea65

isaacroldan added 8 commits May 7, 2024 12:06

Apply prettier format

7aa8124

Update some functions

acc2114

Fix tests

e138c65

Format

799f993

Fix more dynamic imports

e1824a5

Fix more dynamic imports

9fcdbfd

Some cleanup

c723ff9

format

104acce

isaacroldan and others added 4 commits May 7, 2024 13:39

Upgrade oclif and cli-kit

1e7362d

Update cli version in templates

f92ac70

Update minimum oxygen-cli version to bump oclif/core dep

1b8a07d

Dedup package-lock.json

2063edc

frandiox reviewed May 8, 2024

View reviewed changes

This comment was marked as off-topic.

Sign in to view

isaacroldan and others added 11 commits May 9, 2024 16:14

Update packages/cli/package.json

7759f1e

Co-authored-by: Fran Dios <frankdiox@gmail.com>

Create import-utils file to extract all custom imports

f54034c

Merge branch 'main' into prepare-for-bundling-2

1350a0a

Extract all local imports to common file

8df2041

Format code

59491e0

Update package-lock.json

b41e000

Remove unnecessary overrides

181f5a5

Clean unused imports

6b6f20a

Copy virtual routes inside lib

57d2bd3

Merge branch 'main' into prepare-for-bundling-2

c4e0c00

Fix CLI tests

00016fc

frandiox reviewed May 22, 2024

View reviewed changes

frandiox added 2 commits May 22, 2024 17:52

Merge branch 'main' into prepare-for-bundling-2

f1a3167

Fix package-lock

079876b

isaacroldan marked this pull request as ready for review May 22, 2024 09:07

This comment has been minimized.

Sign in to view

frandiox added 2 commits May 23, 2024 12:53

Minor refactor

edef070

Changesets

ac151a7

frandiox approved these changes May 23, 2024

View reviewed changes

frandiox merged commit 65239c7 into main May 23, 2024
13 checks passed

frandiox deleted the prepare-for-bundling-2 branch May 23, 2024 04:40

shopify-github-actions-access bot mentioned this pull request May 23, 2024

[ci] release 2024-04 #2104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare for bundling #2087

Prepare for bundling #2087

isaacroldan commented May 7, 2024 •

edited

shopify bot commented May 7, 2024 •

edited

frandiox left a comment

frandiox May 8, 2024

isaacroldan May 8, 2024

isaacroldan May 8, 2024 •

edited

frandiox May 9, 2024

frandiox May 15, 2024

isaacroldan May 16, 2024

isaacroldan May 16, 2024

frandiox May 8, 2024

isaacroldan May 8, 2024

frandiox May 8, 2024

isaacroldan May 8, 2024

frandiox May 14, 2024

isaacroldan May 14, 2024

frandiox May 15, 2024

This comment was marked as off-topic.

frandiox May 22, 2024

This comment has been minimized.

frandiox left a comment

		const vitePath = require.resolve('vite', {paths: [root]});
		const viteNodePath = joinPath(vitePath, '..', 'dist', 'node', 'index.js');

		`${outDir}/${GENERATOR_TEMPLATES_DIR}/${GENERATOR_STARTER_DIR}`,
		`${outDir}/lib/${GENERATOR_TEMPLATES_DIR}/${GENERATOR_STARTER_DIR}`,

Prepare for bundling #2087

Prepare for bundling #2087

Conversation

isaacroldan commented May 7, 2024 • edited

WHY are these changes introduced?

WHAT is this pull request doing?

HOW to test your changes?

Post-merge steps

Checklist

shopify bot commented May 7, 2024 • edited

frandiox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaacroldan May 8, 2024 • edited

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

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

This comment was marked as off-topic.

Choose a reason for hiding this comment

This comment has been minimized.

frandiox left a comment

Choose a reason for hiding this comment

isaacroldan commented May 7, 2024 •

edited

shopify bot commented May 7, 2024 •

edited

isaacroldan May 8, 2024 •

edited