Expose parseGid and use it internally

frehner commented 1 year ago

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

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

frehner commented 1 year ago

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 commented 1 year ago

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

frehner commented 1 year ago

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?)

frandiox commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

frandiox commented 1 year ago

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

jamalsoueidan commented 1 year ago

Thank you

Shopify / hydrogen