Skip to content

Comments

Reorganizing + fixing hubl auto detection#295

Merged
brandenrodgers merged 7 commits intomasterfrom
br/organizing
Dec 5, 2025
Merged

Reorganizing + fixing hubl auto detection#295
brandenrodgers merged 7 commits intomasterfrom
br/organizing

Conversation

@brandenrodgers
Copy link
Contributor

Some mostly structural updates to the project to make it easier to build on. This makes some fixes to the hubl auto detection functionality that seems to have not been working. The major changes:

  • hubl auto detection should work now (I could have just been testing this incorrectly, but it wasn't working for me originally)
  • Reorganizing the folder structure to be a bit more intuitive
  • Grouping common utils into files in lib/
  • Moving features out of the lib folder
  • Creating a dedicated "Events" folder
  • Turning certain registered commands into standard js functions in areas where they don't need to be registered
  • Cleaning up some dead code
  • Removing some unnecessary deps
  • Organizing imports for all files

fileAssoc[`*.html`] = HUBL_HTML_ID;
fileAssoc[`*.css`] = HUBL_CSS_ID;
filesConfig.update('associations', fileAssoc);
filesConfig.update('associations', fileAssoc, ConfigurationTarget.Workspace);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be explicit here and save the "this project uses hubl" setting at the workspace level.

@@ -4,47 +4,67 @@ import {
window,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were two issues in this. The loop for the hubl detection wasn't reading the first character, and the hubl check logic wasn't running for documents that were already opened in the editor.

"command": "hubspot.remoteFs.hardRefresh",
"title": "Refresh"
},
{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These commands can be deleted from here. This project uses a pattern where we have "events" and "commands".

  • commands - user-facing commands that can be executed via some form of ui interaction
  • events - internal-only "commands" that cannot be directly triggered by a user through the ui

Anything that is purely an event, does not need to be configured here in the package.json file. I cleaned up everything in here that was defined as a command, has a "when: false" clause, and is not referenced anywhere else in the package.json file (i.e. menu options or something else).

loader: 'node-loader',
},
{
test: /\.lyaml$/,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import * as dayjs from 'dayjs';
import { COMMANDS } from '../constants';

export const registerCommands = (context: ExtensionContext) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the patterns of this project, this is actually an "event" instead of a command. But it also isn't leveraged anywhere in the package.json file, so we can remove this completely. It's just a thin wrapper around the context.globalState.update() function. I added two utils to the helpers.ts file, updated the panels/feedback.ts file, and then updated all the code that previously called executeCommand(COMMANDS.GLOBAL_STATE.UPDATE_DELAY) to directly call the global state update function instead.

@@ -0,0 +1,11 @@
import { ExtensionContext } from 'vscode';
Copy link
Contributor Author

@brandenrodgers brandenrodgers Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still ramping up here, but from what I can tell so far this "Events" pattern is adding unnecessary complexity. In most (all?) cases there isn't a need to leverage VSCode's event bus to execute logic like this. AFAIK the primary reason to register events is to expose them as user-facing functionality in the contributes section of the package.json.

I undid some of it (all the terminal logic that checks the installation status for npm and the cli, and also the global state events), but not all of it because some events are pretty deeply rooted.

import { COMMANDS, POLLING_INTERVALS } from '../constants';

export const registerCommands = (context: ExtensionContext) => {
context.subscriptions.push(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an example of "events" that don't need to be events. These aren't referenced anywhere in the package.json file, so there's no reason for them to leverage the register/execute event bus pattern. I ported all of these into the terminal.ts file and now they're called directly as standard js functions

@@ -1,13 +1,3 @@
export const TEMPLATE_ERRORS_TYPES = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed some constants from here that are only used in a single place

@@ -1,11 +1,5 @@
import { dirname } from 'path';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file was operating as a catch-all so I moved some of the utils to dedicated lib files

@@ -0,0 +1,11 @@
import { ExtensionContext } from 'vscode';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly just moving files around for all these providers so they follow the same registration pattern that we're using for panels, commands, and events

Copy link
Contributor

@camden11 camden11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still ramping up with the VScode extension so not entirely sure whats going on 100% of the time here, but nothing stood out to me. Still need to test things out

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to update the repo to use yarn since everything else we own does

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I'll do that in a separate pr since I already have too many changes bundled into this one

Copy link
Contributor

@camden11 camden11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still looking through the code, but when testing I noticed that the "Authenticate Additional HubSpot Account" button doesn't seem to be working. Not sure if that's just an issue with my local setup, but it works as expected in my installed version of the extension

Copy link
Contributor

@camden11 camden11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once again, not really a VSCode expert but code here LGTM. Only issue I can find is the auth button not working

@brandenrodgers
Copy link
Contributor Author

@camden11 good catch. I fixed the key in de9debe

Copy link
Contributor

@camden11 camden11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, auth button is working again! LGTM

@brandenrodgers brandenrodgers merged commit 675a722 into master Dec 5, 2025
1 check passed
@brandenrodgers brandenrodgers deleted the br/organizing branch December 5, 2025 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants