-
Notifications
You must be signed in to change notification settings - Fork 125
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
App watcher - Build extensions #4346
base: main
Are you sure you want to change the base?
Conversation
We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset. |
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success1892 tests passing in 859 suites. Report generated by 🧪jest coverage report action from 196e47e |
const interval = setInterval(() => { | ||
if (emitSpy.mock.calls.length > 0) { | ||
clearInterval(interval) | ||
resolve() | ||
} | ||
}, 100) | ||
// Wait max 3 seconds, if not resolved, reject. | ||
setTimeout(() => { | ||
clearInterval(interval) | ||
reject(new Error('Timeout waiting for emitSpy to be called')) | ||
}, 3000) |
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 wonder if this kind of thing would be a bit easier to read:
const interval = setInterval(() => { | |
if (emitSpy.mock.calls.length > 0) { | |
clearInterval(interval) | |
resolve() | |
} | |
}, 100) | |
// Wait max 3 seconds, if not resolved, reject. | |
setTimeout(() => { | |
clearInterval(interval) | |
reject(new Error('Timeout waiting for emitSpy to be called')) | |
}, 3000) | |
const initialTime = Date.now() | |
while (emitSpy.mock.calls.length === 0) { | |
if (Date.now() - initialTime < 3000) { | |
await sleep(100) | |
} else { | |
reject(new Error('Timeout waiting for emitSpy to be called')) | |
} | |
} | |
resolve() |
Note we might have something about not awaiting in loops 😢
} | ||
|
||
async start() { | ||
// If there is a previous build folder, delete it | ||
if (fileExistsSync(this.buildOutputPath)) await rmdir(this.buildOutputPath, {force: true}) |
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.
Why not:
if (fileExistsSync(this.buildOutputPath)) await rmdir(this.buildOutputPath, {force: true}) | |
if (await fileExists(this.buildOutputPath)) await rmdir(this.buildOutputPath, {force: true}) |
It's an async function anyway, may as well use nonblocking operations.
WHY are these changes introduced?
App watcher now has the responsibility of building extensions when they change.
For extensions that use ESBuild, it handles that using using contexts.
Having a unified place for watching and building extensions during dev is very convenient, as the events from here can be used by the different processes running during
dev
:WHAT is this pull request doing?
UpdatedSource
event type for the watcher..shopify
directory by default.dev
flow to use this new feature.How to test your changes?
Having an app with different extensions (specially ui-extensions and functions):
pnpm shopify demo watcher --path <path_to_app>
.shopify/bundle
directory, you should see all the built extensions.Alternative: Having an org in app-management, run
dev
with the new API.Post-release steps
Measuring impact
How do we know this change was effective? Please choose one:
Checklist