-
Notifications
You must be signed in to change notification settings - Fork 158
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
Extract common bundling functions for dev and deploy #5580
base: main
Are you sure you want to change the base?
Extract common bundling functions for dev and deploy #5580
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
We detected some changes at packages/*/src and there are no updates in the .changeset. |
f957c43
to
6a9949f
Compare
export async function writeManifestToBundle(app: AppInterface, bundlePath: string) { | ||
const appManifest = await app.manifest() | ||
const manifestPath = joinPath(bundlePath, 'manifest.json') | ||
await writeFile(manifestPath, JSON.stringify(appManifest, null, 2)) | ||
} |
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.
Is it worth doing this now? because with incremental-dev we are going to have different ways to build the manifest for dev and deploy, and it won't be included in the zip file during dev
😅
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.
Here are the changes I made for incremental-dev in the dev-session file:
https://app.graphite.dev/github/pr/Shopify/cli/5564/Incremental-dev#file-packages/app/src/cli/services/dev/processes/dev-session/dev-session.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.
What if we send the manifest instead of the app? So that we can send different manifests
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.
But we don't want to write the manifest to a file for the dev
case right?
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.
Aren't we going to do the same in both places? I don't think it's a good idea to keep two different bundle structures.
6a9949f
to
d4e959f
Compare
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success2171 tests passing in 953 suites. Report generated by 🧪jest coverage report action from 02407e7 |
d4e959f
to
02407e7
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/api/partners.d.ts@@ -1,6 +1,6 @@
import { GraphQLVariables, GraphQLResponse, CacheOptions } from './graphql.js';
-import { TypedDocumentNode } from '@graphql-typed-document-node/core';
import { Variables } from 'graphql-request';
+import { TypedDocumentNode } from '@graphql-typed-document-node/core';
/**
* Executes a GraphQL query against the Partners API.
*
|
WHY are these changes introduced?
Small refactor required for #5581
WHAT is this pull request doing?
Extract some common functions for dev and deploy to write the manifest and compress the bundle.
How to test your changes?
Dev and deploy
Measuring impact
How do we know this change was effective? Please choose one:
Checklist