-
Notifications
You must be signed in to change notification settings - Fork 219
Add declarative definitions import service #6651
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
base: shauns/12-05-automatic_rate_limit_restoration_w__admin_api_calls
Are you sure you want to change the base?
Add declarative definitions import service #6651
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
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. |
Coverage report
Test suite run success3489 tests passing in 1402 suites. Report generated by 🧪jest coverage report action from ae8cbbd |
cca80dc to
375e9d7
Compare
ec968cf to
4d6099e
Compare
232d079 to
a61c3ab
Compare
a61c3ab to
bd265d7
Compare
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.
The code is a bit hard to follow, I would try to simplify in general.
Also, I would add some debug messages around to be able to investigate issues from verbose outputs.
I left some random suggestions, but nothing really important.
| "{projectRoot}/src/cli/api/graphql/functions/generated/**/*.ts", | ||
| "{projectRoot}/src/cli/api/graphql/bulk-operations/generated/**/*.ts" | ||
| "{projectRoot}/src/cli/api/graphql/bulk-operations/generated/**/*.ts", | ||
| "{projectRoot}/src/cli/api/graphql/admin/generated/**/*.ts" |
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.
Couldn't we just replace all these with something like this?
"{projectRoot}/src/cli/api/graphql/**/generated/**/*.ts"
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.
Good shout. But let me add as a separate piece of stack -- there could be good reason, nx has its idiosyncrasies.
| "pnpm eslint 'src/cli/api/graphql/functions/generated/**/*.{ts,tsx}' --fix", | ||
| "pnpm eslint 'src/cli/api/graphql/bulk-operations/generated/**/*.{ts,tsx}' --fix" | ||
| "pnpm eslint 'src/cli/api/graphql/bulk-operations/generated/**/*.{ts,tsx}' --fix", | ||
| "pnpm eslint 'src/cli/api/graphql/admin/generated/**/*.{ts,tsx}' --fix" |
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.
Same here
| "{projectRoot}/src/cli/api/graphql/functions/generated/**/*.ts", | ||
| "{projectRoot}/src/cli/api/graphql/bulk-operations/generated/**/*.ts" | ||
| "{projectRoot}/src/cli/api/graphql/bulk-operations/generated/**/*.ts", | ||
| "{projectRoot}/src/cli/api/graphql/admin/generated/**/*.ts" |
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.
And here
packages/app/src/cli/services/generate/shop-import/declarative-definitions.ts
Show resolved
Hide resolved
packages/app/src/cli/services/generate/shop-import/declarative-definitions.ts
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/generate/shop-import/declarative-definitions.ts
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/generate/shop-import/declarative-definitions.ts
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/generate/shop-import/declarative-definitions.ts
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/generate/shop-import/declarative-definitions.ts
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/generate/shop-import/declarative-definitions.test.ts
Show resolved
Hide resolved
bd265d7 to
ac99e52
Compare
ac99e52 to
ae8cbbd
Compare
80c8dcb to
68d8802
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/http.d.ts@@ -18,7 +18,7 @@ type AutomaticCancellationBehaviour = {
} | {
useAbortSignal: AbortSignal | (() => AbortSignal);
};
-type RequestBehaviour = NetworkRetryBehaviour & AutomaticCancellationBehaviour;
+export type RequestBehaviour = NetworkRetryBehaviour & AutomaticCancellationBehaviour;
export type RequestModeInput = PresetFetchBehaviour | RequestBehaviour;
/**
* Specify the behaviour of a network request.
packages/cli-kit/dist/public/node/api/graphql.d.ts@@ -47,6 +47,7 @@ export type GraphQLRequestDocOptions<TResult, TVariables> = GraphQLRequestBaseOp
}>>;
variables?: TVariables;
unauthorizedHandler?: UnauthorizedHandler;
+ autoRateLimitRestore?: boolean;
};
export interface GraphQLResponseOptions<T> {
handleErrors?: boolean;
|

WHY are these changes introduced?
To enable importing metafield and metaobject definitions from a shop into a TOML configuration file, making it easier for developers to migrate existing custom data structures into their app configuration.
The service loads metafield definitions across all owner types, as well as metaobject definitions. These are then converted into their TOML equivalent, and for finessed rendering, a collection of patches to apply to control TOML formatting.
This service is not exposed within this PR.