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

Use TOML configuration for function build path #4622

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

andrewhassan
Copy link
Contributor

@andrewhassan andrewhassan commented Oct 9, 2024

#gsd:40977

WHY are these changes introduced?

Fixes https://github.com/Shopify/shopify-functions/issues/445

WHAT is this pull request doing?

Function developers have the option of specifying a custom build path for their Wasm file. This PR updates the CLI code to read from that path if it's specified, otherwise use the default path of dist/index.wasm (described here).

How to test your changes?

  1. Create a Rust function with a specific build path (e.g. target/wasm32-wasi/release/my-function.wasm)
  2. Run shopify app deploy

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

Copy link
Contributor

github-actions bot commented Oct 9, 2024

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

  • Themes: @shopify/advanced-edits
  • UI extensions: @shopify/ui-extensions-cli
    • Checkout UI extensions: @shopify/checkout-ui-extensions-api-stewardship
  • Hydrogen: @shopify/hydrogen
  • Other: @shopify/app-inner-loop

@andrewhassan
Copy link
Contributor Author

/snapit

Copy link
Contributor

github-actions bot commented Oct 9, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
72.68% (+0.01% 🔼)
8541/11752
🟡 Branches
69.65% (+0.01% 🔼)
4197/6026
🟡 Functions 71.73% 2207/3077
🟡 Lines
73.01% (+0.01% 🔼)
8085/11074

Test suite run success

1943 tests passing in 876 suites.

Report generated by 🧪jest coverage report action from 083aaaf

Copy link
Contributor

github-actions bot commented Oct 9, 2024

🫰✨ Thanks @andrewhassan! Your snapshot has been published to npm.

Test the snapshot by intalling your package globally:

pnpm i -g @shopify/[email protected]

After installing, validate the version by running just shopify in your terminal
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

@andrewhassan
Copy link
Contributor Author

/snapit

@andrewhassan andrewhassan marked this pull request as ready for review October 9, 2024 19:27
@andrewhassan andrewhassan requested a review from a team as a code owner October 9, 2024 19:27
@andrewhassan andrewhassan requested review from jamieguerrero and gordonhirsch and removed request for a team October 9, 2024 19:27
Copy link
Contributor

github-actions bot commented Oct 9, 2024

We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

Copy link
Contributor

github-actions bot commented Oct 9, 2024

🫰✨ Thanks @andrewhassan! Your snapshot has been published to npm.

Test the snapshot by intalling your package globally:

pnpm i -g @shopify/[email protected]

After installing, validate the version by running just shopify in your terminal
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

Copy link
Contributor

@DuncanUszkay1 DuncanUszkay1 left a comment

Choose a reason for hiding this comment

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

Did this feature simply not work before? 🤔 Or is this net-new?

Also- is this something worth testing? Or are we considering our smoke tests to be sufficient here?

@andrewhassan
Copy link
Contributor Author

@DuncanUszkay1 This is a net new bug from this PR, which was shipped yesterday.

Copy link
Contributor

@saga-dasgupta saga-dasgupta left a comment

Choose a reason for hiding this comment

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

Thanks a lot for fixing this!

Copy link
Contributor

@DuncanUszkay1 DuncanUszkay1 left a comment

Choose a reason for hiding this comment

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

Ah, I see now. Thanks for the context

@andrewhassan andrewhassan added this pull request to the merge queue Oct 10, 2024
Merged via the queue into main with commit a27c9b4 Oct 10, 2024
36 checks passed
@andrewhassan andrewhassan deleted the ah.fix-function-build branch October 10, 2024 12:57
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