Skip to content
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

Chore: project structure ts port #1302

Merged
merged 3 commits into from
Dec 16, 2024
Merged

Conversation

brandenrodgers
Copy link
Contributor

Description and Context

Porting projectStructure to TS. I had to add types for project components which doesn't feel great, but I'm not sure that there's another option. Eventually we should offload this type of thing into the new package that will specialize in interacting with project schema, but in the meantime I think we need to keep this here.

I could also add these types to Local Dev Lib, but I think I'd just be adding more work for us down the road when we eventually start to use the translation lib.

Screenshots

TODO

Who to Notify

function getIsLegacyApp(
appConfig: PublicAppComponentConfigType | PrivateAppComponentConfigType,
appPath: string
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs a return type

}
}
});

return components;
}

function getProjectComponentTypes(components) {
const projectContents = {};
export function getProjectComponentTypes(components: Array<Component>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs a return type

export function getAppCardConfigs(
appConfig: PublicAppComponentConfigType | PrivateAppComponentConfigType,
appPath: string
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs a return type

if (CONFIG_FILES[key] === configFile) {
return key;
}
}
return null;
}

function loadConfigFile(configPath) {
function loadConfigFile(configPath: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs a return type

Copy link
Contributor

@kemmerle kemmerle left a comment

Choose a reason for hiding this comment

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

Code looks good to me; there's no changes in behavior, etc. Just need to add some return types

import { logError } from './errorHandlers/index';

type ComponentTypes = 'private-app' | 'public-app' | 'hubl-theme';
type ValueOf<T> = T[keyof T];
Copy link
Contributor

Choose a reason for hiding this comment

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

We export ValueOf from @hubspot/local-dev-lib/types/Utils so you don't need to redefine it here

function getTypeFromConfigFile(configFile) {
for (const key in CONFIG_FILES) {
function getTypeFromConfigFile(
configFile: ValueOf<typeof CONFIG_FILES>
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this just resolves to string. Which... I think is probably fine but we can just type at that way directly. I messed around with adding an as const to CONFIG_FILES to fix this and it caused its own set of problems

import { logger } from '@hubspot/local-dev-lib/logger';
import { logError } from './errorHandlers/index';

type ComponentTypes = 'private-app' | 'public-app' | 'hubl-theme';
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you already declare the COMPONENT_TYPES object below, you could derive this type from that

type ComponentTypes = ValueOf<typeof COMPONENT_TYPES>;

@brandenrodgers brandenrodgers merged commit 9422cfe into next Dec 16, 2024
1 check passed
@brandenrodgers brandenrodgers deleted the br/project-structure-ts-port branch December 16, 2024 16:41
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.

3 participants