Add appAssets to the websocket app payload#7226
Add appAssets to the websocket app payload#7226melissaluu wants to merge 1 commit into04-08-extract_custom_watch_paths_to_specificationsfrom
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. |
melissaluu
force-pushed
the
04-08-add_appassets_to_the_websocket_app_payload
branch
from
63872c8 to
e80096c
Compare
|
|
||
| const path = joinPath(extension.directory, staticRoot, '**/*') | ||
| return {paths: [path], ignore: []} | ||
| return {paths: [path], ignore: [], assetKey: 'staticRoot'} |
There was a problem hiding this comment.
This was added so that we can use this config in the resolveAppAssets()
We don't have to do this if we're okay with directly using App object to find the appAsset directory
| * Derives app-level asset directories from config extensions that define devSessionWatchConfig | ||
| * with an assetKey. Returns a map of asset key (e.g. 'static_root') to absolute directory path. | ||
| */ | ||
| export function resolveAppAssets(allExtensions: ExtensionInstance[]): Record<string, string> { |
There was a problem hiding this comment.
We can switch this back to using the App object to find the appAssets directory path (i.e. app.configuration.assets.static_root) but this implementation removes hardcoding and specifying the path directly and we grab it from the devSessionWatchConfig instead
| : undefined, | ||
| await setupPreviewableExtensionsProcess({ | ||
| allExtensions: reloadedApp.allExtensions, | ||
| allExtensions: reloadedApp.realExtensions, |
There was a problem hiding this comment.
@isaacroldan is this safe? I asked Claude multiple times to check this and it said that this should be okay since technically reloadedApp.allExtensions can omit config extensions with when includeConfigOnDeploy is set to false
There was a problem hiding this comment.
why do you need this change though?
| outputDebug(`Setting up the UI extensions bundler and file watching...`, payloadOptions.stdout) | ||
|
|
||
| const eventHandler = async ({appWasReloaded, app, extensionEvents}: AppEvent) => { | ||
| const eventHandler = async ({appWasReloaded, app, extensionEvents, appAssetsUpdated}: AppEvent) => { |
There was a problem hiding this comment.
you also have the app here from the AppEvent, this one is better because is the updated app (in case anything changed)
| const baseDir = normalizePath(globPatternBaseDir(watchConfig.paths[0]!)) | ||
| appAssets[watchConfig.assetKey] = baseDir | ||
| } | ||
| return appAssets |
There was a problem hiding this comment.
This solution is very hacky. We are adding the assetKey as a way of generization, but in the way we are polluting the devSessionWatchConfig with a property that doesn't have anything to do with watching, i'll think on a better way to extract this.
| const baseDir = normalizePath(globPatternBaseDir(watchConfig.paths[0]!)) | ||
| appAssets[watchConfig.assetKey] = baseDir | ||
| } | ||
| return appAssets |
There was a problem hiding this comment.
This solution is very hacky. We are adding the assetKey as a way of generization, but in the way we are polluting the devSessionWatchConfig with a property that doesn't have anything to do with watching, i'll think on a better way to extract this.
2 participants
WHY are these changes introduced?
Fixes #0000
WHAT is this pull request doing?
How to test your changes?
Post-release steps
Checklist
pnpm changeset add