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

Remove print extension uniqueness validation #4232

Closed
wants to merge 3 commits into from

Conversation

alfonso-noriega
Copy link
Contributor

@alfonso-noriega alfonso-noriega commented Jul 23, 2024

WHY are these changes introduced?

Remove print extension validation which should live in Core.
issue #1183

NOTE: This shouldn't be merged until core validation is merged in this PR

WHAT is this pull request doing?

Remove the validation as it is implemented in cor in this PR

How to test your changes?

  • Create an app with two print extensions with the same target
  • In the CLI project, checkout the branch 1183-move-cli-validation-to-core
  • In an spin instance, go to shopify shell and checkout the branch 1183-move-cli-validation-to-core
  • Try to deploy the app using the local CLI branch, it should return an error mensioning the extension handles and the target

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

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-management

@alfonso-noriega alfonso-noriega force-pushed the 1183-move-cli-validation-to-core branch from 512aecf to a4562bd Compare July 24, 2024 16:34
Copy link
Contributor

github-actions bot commented Jul 24, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
72.42% (+0.07% 🔼)
7797/10767
🟡 Branches
69.4% (+0.03% 🔼)
3844/5539
🟡 Functions
71.23% (-0.04% 🔻)
2052/2881
🟡 Lines
72.72% (+0.08% 🔼)
7364/10127
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / loader.ts
94.08% (-0.27% 🔻)
85.88% (-0.32% 🔻)
97.85% (-0.15% 🔻)
95.18% (-0.19% 🔻)

Test suite run success

1787 tests passing in 813 suites.

Report generated by 🧪jest coverage report action from a3be938

@alfonso-noriega alfonso-noriega marked this pull request as ready for review July 25, 2024 12:38
Copy link
Contributor

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.

@alfonso-noriega
Copy link
Contributor Author

/snapit

Copy link
Contributor

🫰✨ Thanks @alfonso-noriega! Your snapshot has been published to npm.

Test the snapshot by intalling your package globally:

pnpm i -g @shopify/cli@0.0.0-snapshot-20240725140404

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

@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.

🔥

Remember to remove this type as well

@alfonso-noriega alfonso-noriega force-pushed the 1183-move-cli-validation-to-core branch from a4562bd to f3759f0 Compare July 26, 2024 12:11
@alfonso-noriega alfonso-noriega force-pushed the 1183-move-cli-validation-to-core branch from f3759f0 to a3be938 Compare July 26, 2024 12:31
Copy link
Member

@vividviolet vividviolet left a comment

Choose a reason for hiding this comment

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

I'm good with removing the validation but it would mean they can still dev an app with duplicated print targets even if the backend validation fails. Should we actually be failing the dev process when an error happens when attempting to update the extension version? This would allow us to remove duplicated front end validation on the various Hosts to account for the dev flow and keep all validation on the backend. For example, we have fallbacks for the name being missing even though this can never be true for real extensions.

cc @isaacroldan

@alfonso-noriega
Copy link
Contributor Author

...it would mean they can still dev an app with duplicated print targets even if the backend validation fails. Should we actually be failing the dev process when an error happens when attempting to update the extension version?...

@vividviolet,this won't be an issue anymore once the new consistent dev and dev dashboard projects land as they will create a version on dev?

@isaacroldan
Copy link
Contributor

That's correct.

@vividviolet, what would happen if you dev with duplicated print targets? would the draft update fail? where will it eventually crash?

@vividviolet
Copy link
Member

this won't be an issue anymore once the new consistent dev and dev dashboard projects land as they will create a version on dev?

@alfonso-noriega It wouldn't be an issue in production but still an issue in development.

@vividviolet, what would happen if you dev with duplicated print targets? would the draft update fail?

When you dev with duplicated target the draft update will fail and show and error in the logs. However, the dev process isn't stopped so you can still dev. What I'm suggesting is for us to just fail on validation errors instead of letting them go through with dev using a "broken" extension.

where will it eventually crash?

It doesn't really crash currently because there's extra logic in Web to account for multiple print action extension (I believe we just use the first one). I would like to get rid of the extra logic if possible. We have extra logic for when the name is not populated as well even though it would fail validation.

@isaacroldan
Copy link
Contributor

What I'm suggesting is for us to just fail on validation errors instead of letting them go through with dev using a "broken" extension.

I think it makes sense to stop dev if there is an error in the draft upload.

@alfonso-noriega
Copy link
Contributor Author

alfonso-noriega commented Aug 2, 2024

I think it makes sense to stop dev if there is an error in the draft upload.

@isaacroldan should we take an action on this? I can extend this PR to fail on draft upload error or create a new one. But we should not merge this one until we get that merged.

@vividviolet
Copy link
Member

What I'm suggesting is for us to just fail on validation errors instead of letting them go through with dev using a "broken" extension.

I think it makes sense to stop dev if there is an error in the draft upload.

FYI, here's what happens on a validation error currently. The dev process just continues:

image

cc @alfonso-noriega @cameronbarker

Copy link
Contributor

This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action.
→ If there's no activity within a week, then a bot will automatically close this.
Thanks for helping to improve Shopify's dev tooling and experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants