-
Notifications
You must be signed in to change notification settings - Fork 224
Match by both type and externalType #6332
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
Conversation
Coverage report
Show files with reduced coverage 🔻
Test suite run success3194 tests passing in 1346 suites. Report generated by 🧪jest coverage report action from 25baed5 |
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
|
/snapit |
|
🫰✨ Thanks @isaacroldan! Your snapshot has been published to npm. Test the snapshot by installing your package globally: npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20250828121047Caution After installing, validate the version by running just |
gonzaloriestra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working as expected 👌
| return ( | ||
| remote.type.toLowerCase() === local.graphQLType.toLowerCase() && slugify(remote.title) === slugify(local.handle) | ||
| (remote.type.toLowerCase() === local.graphQLType.toLowerCase() || | ||
| remote.type.toLowerCase() === (local as ExtensionInstance).externalType || | ||
| remote.type.toLowerCase() === (local as ExtensionInstance).type) && | ||
| slugify(remote.title) === slugify(local.handle) | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it a bit confusing with the parenthesis and so on, so I'd simplify with something like this:
| return ( | |
| remote.type.toLowerCase() === local.graphQLType.toLowerCase() && slugify(remote.title) === slugify(local.handle) | |
| (remote.type.toLowerCase() === local.graphQLType.toLowerCase() || | |
| remote.type.toLowerCase() === (local as ExtensionInstance).externalType || | |
| remote.type.toLowerCase() === (local as ExtensionInstance).type) && | |
| slugify(remote.title) === slugify(local.handle) | |
| ) | |
| const remoteType = remote.type.toLowerCase(); | |
| const localExt = local as ExtensionInstance; | |
| const sameType = [localExt.graphQLType?.toLowerCase(), localExt.externalType, localExt.type] | |
| .includes(remoteType) | |
| const sameName = slugify(remote.title) === slugify(local.handle) | |
| return sameType && sameName |
| const sameTypeAndName = (local: LocalSource, remote: RemoteSource) => { | ||
| return ( | ||
| remote.type.toLowerCase() === local.graphQLType.toLowerCase() && slugify(remote.title) === slugify(local.handle) | ||
| (remote.type.toLowerCase() === local.graphQLType.toLowerCase() || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have tests in place already can we add the new case? Otherwise probably fine to skip since this code will be 🔥 soon anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a test 👌
| (remote.type.toLowerCase() === local.graphQLType.toLowerCase() || | ||
| remote.type.toLowerCase() === (local as ExtensionInstance).externalType || | ||
| remote.type.toLowerCase() === (local as ExtensionInstance).type) && | ||
| slugify(remote.title) === slugify(local.handle) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This confused me initially because matching the remote title with the local handle likely wouldn't work in many cases.
Discussed with @isaacroldan and in this circumstance the remote.title actually represents the handle of the module. Makes this difficult to read and grok but should be gone soon so it is probably okay in the short term?
Either way a comment may be helpful for the next person.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a comment 👌
64a55c4 to
25baed5
Compare

WHY are these changes introduced?
Fixes an issue with extension type matching in the context service.
WHAT is this pull request doing?
Match extensions with both
identifier andexternal_identifier to support the case for both Partners and Dev Dash.How to test your changes?
How do we know this change was effective? Please choose one:
Checklist