Expose parseGid and use it internally

[frehner]

WHY are these changes introduced?

Currently we have 2 implementations of parseGid() in separate packages; one of them is a basic implementation, and one is more advanced. On top of that, there's still actually additional work we need to do to be able to parse complex GIDs (see this section on "Parameterized Global IDs" https://shopify.dev/docs/api/usage/gids#parameterized-global-ids) and it would be nice to implement that once and have everywhere updated.

Additionally, there's also a need to use that helper in some work I'm doing for https://github.com/Shopify/hydrogen-internal/issues/36 where I'll need to get the raw Shop ID (not the GID).

Maybe we should also consider parsing any params out of the ID and having them as a separate item in the returned object? For example: https://github.com/Shopify/quilt/blob/42b7dd366d60c0ad0e165e0dd526a4c6a6054046/packages/admin-graphql-api-utilities/src/index.ts#L28-L44

However, if this is actually too much headache, then I don't mind having two implementations. We can just update both at the same time or something. We can still expose parseGid() from hydrogen-react and hydrogen and use it all there, but keep the cli's version as-is.

WHAT is this pull request doing?

Single-sources parseId()

Post-merge steps

Checklist

• I've read the Contributing Guidelines
• I've considered possible cross-platform impacts (Mac, Linux, Windows)
• I've added a changeset if this PR contains user-facing or noteworthy changes
• I've added tests to cover my changes
• I've added or updated the documentation

[cartogram]

I agree with this approach over the alternatives we discussed in Slack. Left a few initial comments, let me know when its ready for a closer look 🥇

[frehner]

Ok, I don't think we have to change cli's dependence on hydrogen-react from peer to dependency; I just needed to update turbo.json to ensure that hydrogen-react was built before cli-hydrogen could be built.

This is ready to go.

[edit] one moment while I validate docs, actually

[frehner]

https://shopify-dev.shopify-dev-minimal-m2kn.anthony-frehner.us.spin.dev/docs/api/hydrogen-react/2023-04/utilities/parsegid

[frehner]

Ah dang; tests are failing because cli now needs hydrogen-react to build. So... I can set it up so that hydrogen-react builds before the cli's tests, or we can just not worry about single-sourcing this function for now. (Or other ideas?)

[lordofthecactus]

Happy with this approach ✅

One pending question for me, is how much does this change the bundle size 🤔

[frandiox]

Ah dang; tests are failing because cli now needs hydrogen-react to build. So... I can set it up so that hydrogen-react builds before the cli's tests, or we can just not worry about single-sourcing this function for now. (Or other ideas?)

@frehner Maybe the CLI should use its own to avoid hard dependencies? I think we should consider even removing the peerDeps in the CLI that are just for types in virtual-routes (they should be devDeps because they're only useful to us) 🤔

I guess ideally we'd have a "common stuff" package where we import this kind of utility from. Maybe not even published, just a place to import things from and then they get bundled in each package. The final app bundle won't have 2 versions of this because the one in the CLI is not used in-app.

Note that hydrogen and hydrogen-react should use the same, I agree there 100%.

[frehner]

Maybe the CLI should use its own to avoid hard dependencies? I think we should consider even removing the peerDeps in the CLI that are just for types in virtual-routes (they should be devDeps because they're only useful to us) 🤔

Honestly that's kind of the direction I'm leaning towards right now -- just letting the cli have its own version.

As far as peer vs dev deps, I'm not sure I know enough context to understand which we should use; if they're only used for virtual-routes, and the developer never opens/edits virtual-routes on their own, and the TS compiler doesn't complain if the types aren't present for virtual-routes, then devdeps seems more correct.

[frandiox]

Honestly that's kind of the direction I'm leaning towards right now -- just letting the cli have its own version.

Ok let's do that then 👍

As far as peer vs dev deps, I'm not sure I know enough context to understand which we should use; if they're only used for virtual-routes, and the developer never opens/edits virtual-routes on their own, and the TS compiler doesn't complain if the types aren't present for virtual-routes, then devdeps seems more correct.

Agree. We can test and change deps in another PR though, no need to block this one.

[jamalsoueidan]

I wish you exposed parseGid so we can also use it in the project :)

[frandiox]

@jamalsoueidan Yeah it's now exposed. It will be available in the next release 👍

[jamalsoueidan]

Thank you