Skip to content
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

Fetch metafield definitions on start-up #597

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aswamy
Copy link
Contributor

@aswamy aswamy commented Nov 15, 2024

What are you adding in this PR?

Part of #502

  • Adding the ability to fetch metafield definitions on VSCode startup
  • Does not check the version of the CLI
  • Timeouts if the CLI does not complete the action

Tophat

Unfortunately, the CLI does not contain the metafield definitions pull command yet. To tophat the change override the path variable to use a locally built version of CLI of the main branch.

E.g.

const  path = '<path-to-cli-repo>/packages/cli/bin/dev.js';

What's next? Any followup issues?

  • Should we be adding .shopify/metafields.json as part of gitignore for themes?

Before you deploy

  • I included a minor bump changeset

const isWin = process.platform === 'win32';

export async function fetchMetafieldDefinitions() {
const path = getShopifyCliPath();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current CLI does not have the ability to pull metafields, so tophat this build the CLI PR here and hardcode this value to the path.

E.g.

const path = '<where-ever-you-cloned-it>/cli/packages/cli/bin/dev.js'

@aswamy aswamy requested a review from charlespwd November 15, 2024 20:46
@aswamy aswamy marked this pull request as ready for review November 18, 2024 20:55
@@ -10,6 +11,7 @@ const fileSystems: Record<string, AbstractFileSystem> = {
};

startServer(connection, new VsCodeFileSystem(connection, fileSystems));
fetchMetafieldDefinitions();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should somehow be injected in startServer's fetchMetafieldsForURI injection.

IIRC the metafields should be loaded per theme root, not per workspace.

I don't 100% know how the shopify cli works and if there's an auth per theme, but I don't think someone running a monorepo will necessarily have the same metafield definitions for shop A & shop B

@aswamy
Copy link
Contributor Author

aswamy commented Nov 20, 2024

@charlespwd Should we also only do this if we're working with themes, and not theme app extensions?

@charlespwd
Copy link
Contributor

Should we also only do this if we're working with themes, and not theme app extensions?

I think so. At least until we validate we have a solution for them. Can they even use metafields? Probably?

We do have a getModeForURI: Promise<'theme' | 'app'> in the language server, but it depends on how we load the config. Probably going to be fun (not) to use that from the outside. 😓

@aswamy aswamy force-pushed the fetch-metafield-defs-on-start-up branch from dedbbb8 to 2235083 Compare January 2, 2025 22:10
@aswamy aswamy requested a review from a team as a code owner January 2, 2025 22:10
@aswamy
Copy link
Contributor Author

aswamy commented Jan 2, 2025

Update

  • Made sure we only fetch metafields using CLI if it's a theme, not a theme app or anything else
  • Made sure we only fetch metafields using CLI when called from node environment
  • Fetch metafield definitions for each workspace folder + any folder that get's added dynamically to the workspace

@aswamy aswamy force-pushed the fetch-metafield-defs-on-start-up branch from 2235083 to cbdffd4 Compare January 2, 2025 22:16
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.

2 participants