Skip to content

lock graphql-tools/url-loader #30

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

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

lopert
Copy link
Contributor

@lopert lopert commented Feb 11, 2025

Follow up to #27

When we bumped @graphql-codegen/cli, our packages ends up bumping the following:

npm list graphql-ws
@shopify/[email protected] /Users/lopert/src/github.com/Shopify/shopify-function-javascript
└─┬ @graphql-codegen/[email protected]
  └─┬ @graphql-tools/[email protected]
    └─┬ @graphql-tools/[email protected]
      └── [email protected]

However, graphql-ws stopped supporting node 18 in v6, but we need to keep support for it for the CLI.

If we lock @graphql-tools/[email protected], as done over here, we get the following list

npm list graphql-ws
@shopify/[email protected] /Users/lopert/src/github.com/Shopify/shopify-function-javascript
└─┬ @graphql-tools/[email protected]
  └─┬ @graphql-tools/[email protected]
    └── [email protected]

Tophat 🎩

Using Node 18.

In this repo

I tried using npm link but was not successful, so instead went with the tarball approach.

npm install
npm pack

In an app directory

Add the tarball to app dependencies.

npm add /path/to/shopify-function-javascript/shopify-shopify_function-1.0.5.tgz

In local CLI

Run the command to generate a new extension. Not sure if this actually needs to be run from a local CLI now that I think about it.

pnpm shopify app generate extension --template order_discounts --flavor vanilla-js --path=/path/to/app/ssf-fix

@lopert lopert requested review from gonzaloriestra, a team, mssalemi and jacobsteves and removed request for a team February 11, 2025 18:19
Copy link
Member

@jacobsteves jacobsteves left a comment

Choose a reason for hiding this comment

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

I did not tophat but lgtm

Copy link

@gonzaloriestra gonzaloriestra left a comment

Choose a reason for hiding this comment

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

Couldn't test it, but LGTM!

@lopert lopert merged commit 20f572f into main Feb 11, 2025
1 check passed
@lopert lopert mentioned this pull request Apr 14, 2025
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.

3 participants