Add TypeScript checking via JSDoc type annotations #8

GrantASL19 · 2025-07-11T19:49:47Z

Note: I’ve copied this as-is from Jigsaw-Code/outline-sdk#473 but currently the build script fails due to unrelated issues (will investigate further and open an issue/PR).

(Let me know if this is something you’re interested in at all -- I like having type safety for the project but I know TypeScript imposes some additional burden on contributors and maintainers.)

I started this because while working on Jigsaw-Code#472 I noticed getYAMLFileConfig had some additional edge cases where undefined could get returned, but figured I could fully implement type safety in wrapper_app_project while I was at it.

Some notes:

Adds a basic tsconfig.json with "strict" enabled, but type checking in basic_navigator_example disabled for now. You can run type checking with a new "typescript:check" script.
Adds .vscode/settings.json to configure VS Code to do type checking and match the existing code style
I’m not super familiar with the config format so I may be making some incorrect assumptions here.

(Let me know if this is something you’re interested in at all -- I like having type safety for the project but I know TypeScript imposes some additional burden on contributors and maintainers.) I started this because while working on Jigsaw-Code#472 I noticed getYAMLFileConfig had some additional edge cases where undefined could get returned, but figured I could fully implement type safety in wrapper_app_project while I was at it. Some notes: * Adds a basic tsconfig.json with "strict" enabled, but type checking in basic_navigator_example disabled for now. You can run type checking with a new "typescript:check" script. * Adds .vscode/settings.json to configure VS Code to do type checking and match the existing code style * I’m not super familiar with the config format so I may be making some incorrect assumptions here.

.vscode/settings.json

tsconfig.json

wrapper_app_project/.scripts/util.mjs

* Move Config type to config.mjs * Move resolveConfiguration to config.mjs, rename to validateAndNormalizeConfig, change argument names for clarity. * Move zip function to zip.mjs, rename to compress, change argument names for clarity.

…ring from resolvedConfig.entryDomain if necessary)

wrapper_app_project/.scripts/config.mjs

# Conflicts: # package-lock.json

…r.config.json.handlebars)

…nfig.entryUrl).hostname}`

…ipts directories to scripts)

fortuna

Looks good, but we need to figure out the vscode stuff

.vscode/settings.json

wrapper_app_project/scripts/zip.mjs

…fig) types; separate Config validation (validateConfig) from InternalConfig generation (makeInternalConfig)

…alerConfigBase64"] for clarity

…iling whitespace, apply to PR files

wrapper_app_project/scripts/build.mjs

wrapper_app_project/scripts/config.mjs

fortuna · 2025-07-17T18:11:43Z

wrapper_app_project/scripts/config.mjs

+ *
+ * @returns {ValidConfig | { error: string }}
+ */
+export function validateConfig(args) {


"config" must be qualified. What is it is that you are configuring?

It looks like the app, so perhaps AppConfig?

But Capacitor already provides an app config, so why do we need this?

Also, is this a build config, or a runtime config? Those are different things.

This is a build config, but the thing being built is the Capacitor project (not the Capacitor app itself).

So technically there are 3 configs:

*BuildConfig: the configuration used to process wrapper_app_project/template into output/${projectName}

capacitor.config.json.handlebars: Capacitor build config

Config.kt.handlebars/Config.swift.handlebars: app runtime configs

Maybe something like MakeConfig would make sense, but only if we renamed the build script to make. And the user could confuse this for the Unix make command?

daniellacosse

Note that I would like for us to either use the GTS cli directly or parts of it. We were discussing this to an extent in the CI pr (#12). Try to use those configurations where you can.

…lidRawBuildConfig, InternalConfig → ResolvedBuildConfig

… .vscode to .gitignore

…pp" appId

daniellacosse · 2025-07-23T17:35:02Z

BTW @GrantASL19 - re-requesting the review at the top lets @fortuna and I know that you're ready for another look.

daniellacosse

I would, at the least, extend the official https://github.com/google/gts/blob/main/tsconfig-google.json

Also, let me know if you need any help with @fortuna's comments.

… .prettierrc.js now extend it)

wrapper_app_project/scripts/build.mjs

+await promisify(exec)(`
+  cd ${APP_TARGET_DIR.replaceAll(/\s+/g, '\\ ')}
+  npm install --no-warnings
+  npx cap sync ${internalConfig.platform}
+`);


GrantASL19 · 2025-07-23T19:34:08Z

@daniellacosse Added GTS in 32519eb. tsconfig.json, .eslintrc.json, and .prettierrc.js now extend it. There are a few config tweaks to support top-level await, import.meta, and .mjs files.

At this point I could convert the .mjs files to TypeScript and run the build with tsx if you’re fine with that (based on your note at the bottom of #12)

daniellacosse

Thanks for adding GTS, I can hook that up in #12 one we merge this! We can rewrite to TS later if needed, prefer to not have to preprocess the scripts if possible.

wrapper_app_project/scripts/config.mjs

fortuna

Multiple config formats is a recipe for headache. Prefer to have a single format here with the derivations dynamically computed and no redundancy.

wrapper_app_project/scripts/config.mjs

…nd returns config. Note: 1. I added config file validation using a combination of ts-json-schema-generator (to generate a JSON schema from the ConfigFile type) and ajv (to validate that the config file matches it). This way we don’t need to include a bunch of custom validation logic for each config property. One trade-off here is that SmartDialerConfig isn’t based on the underlying Go code, so it could fall out of sync. If we convert all the scripts to TypeScript we can replace this with e.g. zod or ts-pattern (which I’m familiar with) by starting with a pattern and deriving both a type and validator function, rather than starting with the type and deriving a validator function. 2. To simplify things I made a decision to only accept two CLI arguments: config (for passing in custom config locations) and platform (to choose build platform without creating redundant platform-specific configs). This sidesteps the issues with e.g. passing in additionalDomains (do we accept a comma-separated list?) and smartDialerConfig (do we accept a JSON string then parse it?). 3. There are still two types: Config and ConfigFile. ConfigFile (used for config file validation) is based on Config, but with derived properties (entryDomain, domainList, and smartDialerConfigBase64) removed, and the remaining properties other than entryUrl optional. I don’t see any way to avoid this unless we require that the config file contains all config properties (including the derived properties). Also note that I renamed these from "BuildConfig*" to "Config*". I think "Config" conveys "app maker config" given the name of the project, and having "build" or "make" in the type name might be more confusing since both could be read as a verb. 4. I tried replacing the derived properties with getters as suggested in [1], but to be compatible with the Handlebars template compilation [2] I think we would need to add a custom toJSON method to the class for the derived properties, which was making the class increasingly harder to read and understand vs. just outputting a config object including the derived properties as I did. IMO this is an acceptable trade-off given the config object isn’t really a user-facing thing, but let me know if there’s a better way here. [1]: #8 (comment) [2]: https://github.com/Jigsaw-Code/outline-app-maker/blob/7fa63ea3539a105e66d8ee43791166ed21c5bef9/wrapper_app_project/scripts/build.mjs#L107-L114

fortuna

Please see my comment on the types of config

wrapper_app_project/scripts/createConfig.mjs

wrapper_app_project/scripts/types.ts

fortuna · 2025-07-30T16:27:36Z

wrapper_app_project/template/android/app/src/main/java/org/getoutline/pwa/Config.kt.handlebars

@@ -1,6 +1,6 @@
 package {{appId}}

 object Config {


Suggested change

object Config {

object RuntimeConfig {

Also need to rename the file and update references

fortuna · 2025-07-30T16:28:00Z

wrapper_app_project/template/ios/App/App/Config.swift.handlebars

@@ -1,8 +1,8 @@
 import Foundation

 struct Config {


Suggested change

struct Config {

struct RuntimeConfig {

fortuna · 2025-07-30T16:28:29Z

wrapper_app_project/template/ios/App/App/Config.swift.handlebars

    static var proxyHost: String = "127.0.0.1"
    static var proxyPort: String = "0"


I don't think we need these in the config. The ios and android configs should probably be the same.

Suggested change

static var proxyHost: String = "127.0.0.1"

static var proxyPort: String = "0"

fortuna · 2025-07-30T16:30:51Z

wrapper_app_project/scripts/build.mjs

+    );
+    await fs.writeFile(
+      destinationFilepath.replace(/\.handlebars$/, ''),
+      template(config),


Define a dedicated type TemplateInput for the template input.
It should be derived from the AppMakerConfig with a clear method, like createTemplateInput(appMakerConfig).

Suggested change

template(config),

template(createTemplateInput(appMakerConfig)),

fortuna · 2025-07-30T16:32:54Z

wrapper_app_project/scripts/types.ts

+
+export type Platform = 'android' | 'ios';
+
+export type Config = {


Let's replace this with an AppMakerConfig that is entirely defined by the user. It should be the file format. Remove ConfigFile.

fortuna · 2025-07-30T16:35:54Z

wrapper_app_project/scripts/build.mjs

+await promisify(exec)(`
+  cd ${APP_TARGET_DIR.replaceAll(/\s+/g, '\\ ')}
+  npm install --no-warnings
+  npx cap sync ${config.platform}
+`);


Need to properly quote in case of spaces:

Suggested change

await promisify(exec)(`

cd ${APP_TARGET_DIR.replaceAll(/\s+/g, '\\ ')}

npm install --no-warnings

npx cap sync ${config.platform}

`);

await promisify(exec)(`

cd "${APP_TARGET_DIR.replaceAll(/\s+/g, '\\ ')}"

npm install --no-warnings

npx cap sync "${config.platform}"

`);

This is not safe: npx cap sync ${config.platform}. You need to shell-escape the parameter, or test that they are in a safe set of values.

The safe way to run commands is to use the exec version that takes the args in a list..

GrantASL19 requested a review from daniellacosse July 11, 2025 19:49

GrantASL19 self-assigned this Jul 11, 2025

GrantASL19 added the enhancement New feature or request label Jul 11, 2025

fortuna reviewed Jul 14, 2025

View reviewed changes

.vscode/settings.json Outdated Show resolved Hide resolved

fortuna reviewed Jul 14, 2025

View reviewed changes

tsconfig.json Outdated Show resolved Hide resolved

tsconfig.json Show resolved Hide resolved

wrapper_app_project/.scripts/util.mjs Outdated Show resolved Hide resolved

wrapper_app_project/.scripts/util.mjs Outdated Show resolved Hide resolved

GrantASL19 added 4 commits July 16, 2025 11:13

Remove "js/ts.implicitProjectConfig.checkJs" setting (unnecessary)

affda93

Exclude output directory from type checking

f42f2fb

Improve file organization and documentation:

b56eac0

* Move Config type to config.mjs * Move resolveConfiguration to config.mjs, rename to validateAndNormalizeConfig, change argument names for clarity. * Move zip function to zip.mjs, rename to compress, change argument names for clarity.

Use inputConfig.appId as resolvedConfig.appId if provided (only infer…

ec43d3b

…ring from resolvedConfig.entryDomain if necessary)

GrantASL19 commented Jul 16, 2025

View reviewed changes

wrapper_app_project/.scripts/config.mjs Outdated Show resolved Hide resolved

GrantASL19 commented Jul 16, 2025

View reviewed changes

wrapper_app_project/.scripts/config.mjs Outdated Show resolved Hide resolved

GrantASL19 added 6 commits July 16, 2025 15:25

Merge branch 'main' into feature/add-typescript-checking

8f15d90

# Conflicts: # package-lock.json

Rename .scripts directories to scripts

61e63e4

In validateAndNormalizeConfig pass through entryUrl (used in capacito…

cc58ab9

…r.config.json.handlebars)

Build into provided output, falling back to `output/${new URL(inputCo…

75355ef

…nfig.entryUrl).hostname}`

Fix type error

d88047e

In tsconfig.json remove hidden directories from include (renamed .scr…

4894831

…ipts directories to scripts)

fortuna reviewed Jul 16, 2025

View reviewed changes

.vscode/settings.json Outdated Show resolved Hide resolved

wrapper_app_project/scripts/zip.mjs Outdated Show resolved Hide resolved

GrantASL19 added 3 commits July 16, 2025 17:03

Separate public (Config/ValidConfig) and internal config (InternalCon…

f740949

…fig) types; separate Config validation (validateConfig) from InternalConfig generation (makeInternalConfig)

Rename InternalConfig["smartDialerConfig"] to InternalConfig["smartDi…

808ca62

…alerConfigBase64"] for clarity

Add VS Code configs to auto-format with final newlines and remove tra…

56d65e5

…iling whitespace, apply to PR files

fortuna reviewed Jul 17, 2025

View reviewed changes

wrapper_app_project/scripts/build.mjs Outdated Show resolved Hide resolved

wrapper_app_project/scripts/build.mjs Outdated Show resolved Hide resolved

fortuna reviewed Jul 17, 2025

View reviewed changes

daniellacosse reviewed Jul 17, 2025

View reviewed changes

daniellacosse mentioned this pull request Jul 17, 2025

feat: add issue configuration and CI checks #12

Merged

GrantASL19 added 4 commits July 18, 2025 15:09

Rename types and variables: Config → RawBuildConfig, ValidConfig → Va…

9e1bdd0

…lidRawBuildConfig, InternalConfig → ResolvedBuildConfig

Refactor validateRawBuildConfig into isValidRawBuildConfig type guard

e13a8fc

Improve type descriptions

a210e9b

Rename .vscode/settings.json to .vscode.suggestion/settings.json; add…

42f83cd

… .vscode to .gitignore

Validate that rawBuildConfig.entryUrl is a valid URL; add fallback "a…

a0ba7ce

…pp" appId

daniellacosse requested review from daniellacosse and fortuna July 23, 2025 17:34

daniellacosse requested changes Jul 23, 2025

View reviewed changes

Add https://github.com/google/gts (tsconfig.json, .eslintrc.json, and…

32519eb

… .prettierrc.js now extend it)

github-advanced-security bot found potential problems Jul 23, 2025

View reviewed changes

GrantASL19 requested a review from daniellacosse July 23, 2025 19:34

GrantASL19 mentioned this pull request Jul 23, 2025

Restore open:* scripts #9

Open

daniellacosse previously approved these changes Jul 23, 2025

View reviewed changes

wrapper_app_project/scripts/config.mjs Outdated Show resolved Hide resolved

fortuna requested changes Jul 23, 2025

View reviewed changes

In resolveBuildConfig replace nested ternary with if...else statements

f15f821

GrantASL19 dismissed daniellacosse’s stale review via f15f821 July 24, 2025 17:08

GrantASL19 added 3 commits July 24, 2025 13:35

Inline DEFAULT_CONFIG into rawBuildConfig

a2059f0

Rename getYAMLBuildConfig → yamlBuildConfigToObject

7fa63ea

fortuna requested changes Jul 28, 2025

View reviewed changes

wrapper_app_project/scripts/createConfig.mjs Show resolved Hide resolved

wrapper_app_project/scripts/types.ts Show resolved Hide resolved

wrapper_app_project/scripts/types.ts Show resolved Hide resolved

wrapper_app_project/scripts/types.ts Show resolved Hide resolved

fortuna requested changes Jul 30, 2025

View reviewed changes

GrantASL19 added 2 commits July 30, 2025 12:36

Replace SmartDialerConfig type with object

0265c46

Add copyright headers

303a6e0

fortuna reviewed Jul 30, 2025

View reviewed changes

		static var proxyHost: String = "127.0.0.1"
		static var proxyPort: String = "0"

	template(config),
	template(createTemplateInput(appMakerConfig)),


		export type Platform = 'android' \| 'ios';

		export type Config = {

Uh oh!

Add TypeScript checking via JSDoc type annotations #8

Are you sure you want to change the base?

Add TypeScript checking via JSDoc type annotations #8

Uh oh!

Conversation

GrantASL19 commented Jul 11, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fortuna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniellacosse left a comment

Choose a reason for hiding this comment

Uh oh!

daniellacosse commented Jul 23, 2025

Uh oh!

daniellacosse left a comment

Choose a reason for hiding this comment

Uh oh!

Check warning

Uh oh!

Uh oh!

GrantASL19 commented Jul 23, 2025

Uh oh!

daniellacosse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fortuna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fortuna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!