-
Notifications
You must be signed in to change notification settings - Fork 71
Revamp local dev to support websocket server for UI #1479
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
Conversation
| } | ||
| }); | ||
|
|
||
| handleExit(({ isSIGHUP }) => localDevProcess.stop(!isSIGHUP)); |
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.
LocalDevProcess represents the current local dev session (similar to the old LocalDevManager). Everything that interacts with the LocalDevProcess now lives outside of it and uses public methods to make necessary changes to the process.
This includes
- Watcher
- Keypress listener
- In the future, the websocket server
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'd add a note with that explanation directly into the code.
| @@ -0,0 +1,193 @@ | |||
| import { fetchAppInstallationData } from '@hubspot/local-dev-lib/api/localDevAuth'; | |||
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.
This is a wrapper around the UIEDevModeInterface and includes all app-specific logic that previously lived in LocalDevManager. This lets us keep LocalDevProcess component agnostic
| localDevState: LocalDevState; | ||
| logger: LocalDevLogger; | ||
| }; | ||
|
|
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.
DevServerManager is no longer a singleton. It now has access to the LocalDevState from the currently running process, and in the future, can pass that directly to dev servers
| await Promise.all(this.devServers.map(devServer => callback(devServer))); | ||
| } | ||
|
|
||
| async setup({ |
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.
These lifecycle methods don't need to accept arguments anymore, since they use the LocalDevState
| class LocalDevLogger { | ||
| private state: LocalDevState; | ||
| private mostRecentUploadWarning: string | null; | ||
| private uploadWarnings: string[]; |
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.
This class handles all logging related to LocalDevProcess. It exists primarily to keep the more complex logging logic out of LocalDevProcess but can also be passed to dev servers to give them control over logs related to the process (see below)
| } | ||
| } | ||
|
|
||
| addUploadWarning(warning: string): void { |
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.
This allows dev servers to add specific upload warnings that the logger will then log whenever the upload warning appears. Currently used when apps in a project have production installs, but in the future, other dev servers could also add messages here. For now, it lets us keep the LocalDevLogger component agnostic
| @@ -1,557 +0,0 @@ | |||
| import path from 'path'; | |||
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.
Broken up into LocalDevProcess, LocalDevLogger, LocalDevWatcher, and AppDevModeInterface
| env: Environment; | ||
| }; | ||
|
|
||
| class LocalDevProcess { |
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.
This class has two primary purposes:
- Store
LocalDevStateand pass it to other objects that need access to it (namely the dev servers and logger) - Expose methods that allow interaction with the local dev process by external sources
| initialProjectNodes, | ||
| env, | ||
| }: LocalDevProcessConstructorOptions) { | ||
| this.state = { |
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.
It could make sense for LocalDevState to be its own class in the future, especially if dev servers will be mutating it. For now, I think this object will suffice
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 think it might be a good idea to make the object immutable so that the other dev servers can't change it. We would want to be the source of truth for this information and probably wouldn't want it to change.
| intermediateRepresentation.intermediateNodesIndexedByUid; | ||
| } | ||
| } | ||
|
|
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.
In later PRs, will add more methods that allow the LocalDevUIWebsocketServer to interact with the LocalDevProcess (for example, an upload method)
|
|
||
| const LOG_PREFIX = '[LocalDevUIWebsocketServer] '; | ||
|
|
||
| class LocalDevUIWebsocketServer { |
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.
This is still WIP - started working on it before I realized that the rest of the work in this PR was necessary. But figured it made sense to keep it in here and add to it in following PRs
| unlinkDir: 'unlinkDir', | ||
| }; | ||
|
|
||
| class LocalDevWatcher { |
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.
This is a good example of how external sources can interact with the LocalDevProcess
| const portData = await requestPorts([{ instanceId: SERVER_INSTANCE_ID }]); | ||
| const port = portData[SERVER_INSTANCE_ID]; | ||
|
|
||
| this._server = new WebSocketServer({ port }); |
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.
If we want, we can attach this WebSocketServer to the express server that we already use for the port manager. This would limit the number of ports we use on the users machine and allow us to have a reliable fixed port.
https://github.com/websockets/ws?tab=readme-ov-file#external-https-server
lib/constants.ts
Outdated
| UNIFIED_APPS: 'Developers:UnifiedApps:PrivateBeta', | ||
| } as const; | ||
|
|
||
| export const LOCAL_DEV_UI_WEBSOCKET_MESSAGE_TYPES = { |
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.
Remember to remove these constants, as well.
|
|
||
| // Avoid logging the warning to the console if it is currently the most | ||
| // recently logged warning. We do not want to spam the console with the same message. | ||
| if (warning !== this.mostRecentUploadWarning) { |
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.
We repeat this condition twice--wrap it in a function to DRY the code up a bit?
| } | ||
| } | ||
|
|
||
| fileChangeError(e: unknown): void { |
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.
You could replace the next four functions with a single function like this:
private handleError(
e: unknown,
translationFn: (message: string) => string
): void {
if (this.state.debug) {
logger.error(e);
}
uiLogger.error(translationFn(e instanceof Error ? e.message : ''));
}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 call!
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 only had minor comments. Overall, I think this is a major improvement for local dev--the modular code is much easier to understand now. Ping me when you're finished making changes and I can give it a final ![]()
One other little comment: You might want to rename the PR given that the websocket work has been removed.
|
|
||
| // Avoid logging the warning to the console if it is currently the most | ||
| // recently logged warning. We do not want to spam the console with the same message. | ||
| if (warning !== this.mostRecentUploadWarning) { |
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.
Should we empty the this.uploadWarnings array when we log? Otherwise it will keep logging the same errors every time a new error is added and this method is called
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.
Yeah good question. It's set up so it won't log exactly the same error twice but if there's a second one added it'll go off.... idk, I could see going either way with this, it could be annoying but it could also help prevent people from missing important errors.
If the hypothetical future dev serves work the way the apps dev server does, these messages will be added on startup and only logged once when files are changed, so it wouldn't be a problem then. Maybe makes sense to cross that bridge when we get there
| } | ||
|
|
||
| addUploadWarning(warning: string): void { | ||
| this.uploadWarnings.push(warning); |
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.
Maybe this can be a set so we can de-dupe the warnings
| langFunction: (message: string) => string | ||
| ): void { | ||
| if (this.state.debug) { | ||
| logger.error(e); |
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.
Should this be logger.debug?
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.
Previously, LocalDevManagerV2 was using this pattern with using logger.error, I assume to make errors more visible when in debug mode. I decided to stick with that pattern
|
|
||
| uiLogger.log(''); | ||
| uiLogger.log( | ||
| chalk.hex(UI_COLORS.SORBET)( |
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.
Nit: Could we move the color logic in the lang code?
| uiLink( | ||
| lib.LocalDevManager.learnMoreLocalDevServer, | ||
| 'https://developers.hubspot.com/docs/platform/project-cli-commands#start-a-local-development-server' | ||
| ) |
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.
Nit: We could move the uiLink in the lang file
| uiLink( | ||
| lib.LocalDevManager.viewProjectLink, | ||
| getProjectDetailUrl( | ||
| this.state.projectConfig.name, | ||
| this.state.targetProjectAccountId | ||
| ) || '' |
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.
Nit: We could move the uiLink into the lang file
| noComponents: 'There are no components in this project.', | ||
| betaMessage: 'HubSpot projects local development', | ||
| learnMoreLocalDevServer: 'Learn more about the projects local dev server', | ||
| learnMoreLocalDevServer: uiLink( |
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.
Are these translations in the en.lyaml file too? Would it make sense to remove them as a part of this, or should we wait and do that later once everything is ported?
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've been operating under the assumption that we'd just delete the entire file at the end, so haven't been removing things from it. I don't think there's really any need to, especially since its easy to look up what we haven't ported to the new system by searching for i18n(
brandenrodgers
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.
I tested out a handful of different edge cases and this seems to work the same as the existing setup 🎉
Description and Context
This revamps the local dev flow for unified apps to make it more flexible and modular. Since I've walked everyone through (and left inline comments to explain) I won't go into too much detail here, but the primary purpose of this PR is to separate the many responsibilities of the old
LocalDevManagerV2into discrete classes.While this makes a lot of code changes, the actual UX of running
hs project devshould not change. Appreciate any help with testing to confirm that's true!TODO
LocalDevStateto be immutable but going to do that in a separate PR as well (this is safe for now since the dev servers don't interact directly withLocalDevStateWho to Notify
@brandenrodgers @joe-yeager @kemmerle