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

Clear cache on app init #4186

Merged
merged 2 commits into from
Jul 11, 2024
Merged

Clear cache on app init #4186

merged 2 commits into from
Jul 11, 2024

Conversation

gonzaloriestra
Copy link
Contributor

WHY are these changes introduced?

If you create a new project in a previously used folder, it doesn't run link because the folder may be cached, so the TOML doesn't get updated.

WHAT is this pull request doing?

Clear cache on app init

How to test your changes?

  • p shopify app init --name test-project --template none
  • p shopify app dev --path test-project
  • cat ~/Library/Preferences/shopify-cli-app-nodejs/config.json | grep test-project -A 3 => The cache should be there
  • rm -r test-project
  • p shopify app init --name test-project --template none
  • cat ~/Library/Preferences/shopify-cli-app-nodejs/config.json | grep test-project -A 3 => No cache
  • p shopify app dev --path test-project
  • cat test-project/shopify.app.toml => The TOML should be populated

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

Copy link
Contributor

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
72.37% (+0.01% 🔼)
7551/10434
🟡 Branches
69.01% (+0.02% 🔼)
3719/5389
🟡 Functions 71.37% 1997/2798
🟡 Lines
72.68% (+0.01% 🔼)
7131/9811

Test suite run success

1736 tests passing in 800 suites.

Report generated by 🧪jest coverage report action from 00d80ea

@isaacroldan
Copy link
Contributor

/snapit

Copy link
Contributor

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

Test the snapshot by intalling the CLI 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

@isaacroldan isaacroldan left a comment

Choose a reason for hiding this comment

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

Tested and works 👌

Copy link
Contributor

@amcaplan amcaplan 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 a bit concerned about this approach, as it creates an implicit coupling between the app package's implementation of local storage and this package. If app were ever to change, this would have to be changed, but nothing would make that obvious.

I suspect this is going to be a continuing trend, where create-app needs to reference some general app ideas, and a generalized solution would be worthwhile.

I don't think this information belongs in cli-kit, app is still the right place, but maybe we should consider exporting certain shared concepts.

In the past, create-app couldn't assume app was present, but now with global CLI it should always be present, right? So if we had an export, we could do something like:

import {clearCachedAppInfo} from '@shopify/app/local-storage'

// ...

// Remove cache from previous projects in the same directory
await clearCachedAppInfo(outputDirectory)

which would make the connection more explicit.

I presume this wouldn't work exactly this way with bundled CLI, so maybe @isaacroldan could help clarify if this approach is possible?

If this is going to be more trouble than it's worth, I'm OK with merging as is, since it's a rare bug anyway, so the effects of breakage are limited. But I think it would be nice to solve this in a clean, repeatable way.

@isaacroldan
Copy link
Contributor

Agree that is not the ideal approach having the local storage defined in two places but thought that the simplification was worth it.

create-app currently doesn't depend on the app package, so it is not included in its bundle (we have two bundles, one for @shopify/cli and one for @shopify/create-app). If we were to add this dependency, my guess is that esbuild will be smart enough to only include that specific file from app and nothing else but we'd need to try

@gonzaloriestra
Copy link
Contributor Author

Good point, I had the same concern. Let me check if adding the dependency doesn't include everything.

@isaacroldan
Copy link
Contributor

There is another problem, app depends on create-app to import and share the init command. Not sure if that circular dependency would be a problem 😬

@gonzaloriestra
Copy link
Contributor Author

Yes, it is 😅:

Could not execute command because the task graph has a circular dependency

create-app:build --> app:build --> create-app:build

@gonzaloriestra
Copy link
Contributor Author

I can't find a better solution now... I've created a new issue here, so I'm merging and hopefully we will revisit this soon.

@gonzaloriestra gonzaloriestra added this pull request to the merge queue Jul 11, 2024
Merged via the queue into main with commit 74e88d3 Jul 11, 2024
@gonzaloriestra gonzaloriestra deleted the clear-cache-on-app-init branch July 11, 2024 10:55
@isaacroldan
Copy link
Contributor

The solution would be to move the init command to app and import it in create-app, but that's a bigger task

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