Remove redundant calls to normalizeStoreFqdn

[EvilGenius13]

WHY are these changes introduced?

While looking into a separate issue, I noticed that we make a lot of calls to normalizeStoreFqdn in certain commands. It's job is to normalize the store name in the CLI.

WHAT is this pull request doing?

Removed extra calls to normalizeStoreFqdn as it's originally called very early in the command process. This is the flow before fixing.

1. Flag Parsing (packages/theme/src/cli/flags.ts)
   ↓ User input: "mystore" or "--store=mystore" or env var
   ↓ normalizeStoreFqdn(input)

2. ensureThemeStore 
   ↓ Returns already-normalized store from step 1

3. ensureAuthenticatedThemes
   ↓ Called normalizeStoreFqdn again

4. Admin API calls (adminRequest, adminRequestDoc)
   ↓ Called normalizeStoreFqdn on session.storeFqdn

Images below include a console.log to illustrate the amount of calls.
Before:

After:

How to test your changes?

I would love a 🎩 in case I missed something.

• Pull down the branch
• Build the branch
• Run commands in different ways:
  • theme push
  • theme push -e my_store
  • theme push --store my_store <- omit the myshopify.com to see if it still adds it properly
  • try it with dev or pull

Post-release steps

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

[github-actions]

Coverage report

St.❔	Category	Percentage	Covered / Total
🟡	Statements	78.58% (+0.01% 🔼)	13479/17153
🟡	Branches	72.53% (+0.04% 🔼)	6563/9049
🟡	Functions	78.77%	3507/4452
🟡	Lines	78.95% (+0.01% 🔼)	12743/16141

Test suite run success

3228 tests passing in 1350 suites.

Report generated by 🧪jest coverage report action from a883d5b

[graygilmore]

Where does the initial normalizeStoreFqdn take place? Can we guarantee that it always runs before this?

[EvilGenius13]

The first normalizeStoreFqdn happens at flag parsing. It grabs any store env var whether in a toml or passed in such as --store. It happens before any other part of a command would require it.

The next time it's called is ensureThemeStore in which it takes that value and caches it in the local config JSON.

If a brand new user were to install the CLI today, their first run always requires them to pass a store to login initially so it will save that store data and be used when someone calls a theme command without passing a store.

[karreiro]

Clever performance optimization, @EvilGenius13 🔥 Thank you!

[katiebauchman]

Thanks for removing the redundant normalizeStoreFqdn calls, Josh!

I tested the PR locally with various scenarios:

• theme push --store mystore → Works correctly, normalizes to mystore.myshopify.com
• theme push --store mystore.myshopify.com → Works correctly, no double-normalization
• All test scenarios from the PR description passed successfully

Additional Finding

During testing, I added debug logging and noticed normalizeStoreFqdn is still called twice:

• First with "mystore" (correctly adds .myshopify.com during flag parsing)
• Then again with "mystore.myshopify.com" (the already-normalized value)

The second call happens in ensureAuthenticated (packages/cli-kit/src/private/node/session.ts:181) and doesn't change the value since it's already normalized. This seems like another redundant call that could be removed for completeness.

[DEBUG] normalizeStoreFqdn called with: "mystore"
Trace: [DEBUG] Call stack:
    at normalizeStoreFqdn (file:///packages/cli-kit/dist/public/node/context/fqdn.js:140:13)
    at Object.parse (file:///packages/theme/dist/cli/flags.js:32:33)
    ... (in flag parsing)

[DEBUG] normalizeStoreFqdn called with: "mystore.myshopify.com"
Trace: [DEBUG] Call stack:
    at normalizeStoreFqdn (file:///packages/cli-kit/dist/public/node/context/fqdn.js:140:13)
    at ensureAuthenticated (file:///packages/cli-kit/dist/private/node/session.js:87:43)
    at async ensureAuthenticatedAdmin (file:///packages/cli-kit/dist/public/node/session.js:105:20)

[EvilGenius13]

Thanks for removing the redundant normalizeStoreFqdn calls, Josh!

I tested the PR locally with various scenarios:

• theme push --store mystore → Works correctly, normalizes to mystore.myshopify.com
• theme push --store mystore.myshopify.com → Works correctly, no double-normalization
• All test scenarios from the PR description passed successfully

Additional Finding

During testing, I added debug logging and noticed normalizeStoreFqdn is still called twice:

• First with "mystore" (correctly adds .myshopify.com during flag parsing)
• Then again with "mystore.myshopify.com" (the already-normalized value)

The second call happens in ensureAuthenticated (packages/cli-kit/src/private/node/session.ts:181) and doesn't change the value since it's already normalized. This seems like another redundant call that could be removed for completeness.

[DEBUG] normalizeStoreFqdn called with: "mystore"
Trace: [DEBUG] Call stack:
    at normalizeStoreFqdn (file:///packages/cli-kit/dist/public/node/context/fqdn.js:140:13)
    at Object.parse (file:///packages/theme/dist/cli/flags.js:32:33)
    ... (in flag parsing)

[DEBUG] normalizeStoreFqdn called with: "mystore.myshopify.com"
Trace: [DEBUG] Call stack:
    at normalizeStoreFqdn (file:///packages/cli-kit/dist/public/node/context/fqdn.js:140:13)
    at ensureAuthenticated (file:///packages/cli-kit/dist/private/node/session.js:87:43)
    at async ensureAuthenticatedAdmin (file:///packages/cli-kit/dist/public/node/session.js:105:20)

I did some digging and tried some edits. At this current moment I am going to leave it in.
in packages/store/src/apis/admin/index.ts it looks like we have a call that would use a shop domain that may not be normalized. There are some tests that act like it as well in session.test.ts. I don't have full context on the full CLI ecosystem, but although Theme commands would be fine without this additional call, this may affect usage in certain app commands, or the API itself that gets passed to the other packages.

Please let me know if you feel differently about this! I am also open to opinions on this. I just didn't want to start changing more logic outside the theme command boundaries.

[katiebauchman]

Thanks for further digging into this! The changes make sense - removing the normalizations in adminRequest, adminRequestDoc, and ensureAuthenticatedThemes since the store is already normalized during flag parsing. I agree and think it's prudent to keep the normalization in ensureAuthenticated since other packages may pass unnormalized stores to ensureAuthenticatedAdmin. Nice performance improvement that avoids any breaking changes. LGTM! 🚢