Skip to content

Use native node for running the bot#562

Open
JstnMcBrd wants to merge 12 commits into
mainfrom
mcb/native-node-ts
Open

Use native node for running the bot#562
JstnMcBrd wants to merge 12 commits into
mainfrom
mcb/native-node-ts

Conversation

@JstnMcBrd
Copy link
Copy Markdown
Contributor

Follow up to #541

Modernizes and simplifies our setup

Changes

  • Fixed the required node version
    • Type-stripping is not unflagged in all version of Node 23, so it should be excluded
  • Rewrote all imports from .js to .ts
  • Enforced erasableSyntaxOnly
    • This means not using enums - only downside
  • Pointed node directly to src/main.ts
  • Removed the build step
  • Replaced it with a type-checking script
  • Removed all references to dist
  • Updated the Dockerfile to reflect build changes
  • Excluded test files from type-checking and typed linting
    • There's always been a ton of unaddressed type errors in our test files, but the build step never caught them because it excluded test files
    • The type-checking step doesn't exclude test files, so all the type errors are visible now
    • There's way too many to fix right now, so I'm saving it for later

@JstnMcBrd JstnMcBrd requested a review from a team May 13, 2026 11:47
@JstnMcBrd JstnMcBrd self-assigned this May 13, 2026
@JstnMcBrd JstnMcBrd added the enhancement New feature or request label May 13, 2026
@JstnMcBrd
Copy link
Copy Markdown
Contributor Author

The branch protection rules need to be updated to expect CI / Type check instead of CI / Build.

Comment thread package.json Outdated
Comment thread Dockerfile

ENV NODE_ENV=production

RUN apt-get update -y
Copy link
Copy Markdown
Contributor

@AverageHelper AverageHelper May 13, 2026

Choose a reason for hiding this comment

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

Not sure about removing this line. Shouldn't we update the package lists before trying to install packages?

Copy link
Copy Markdown
Contributor Author

@JstnMcBrd JstnMcBrd May 22, 2026

Choose a reason for hiding this comment

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

Only when you want the most recent package versions, which is unnecessary for our use case (simply installing any version of openssl). Relying on auto-updates from node:24-slim is sufficient.

Each line of a Dockerfile is an additional layer, so I figure it's better to cut it.

@AverageHelper
Copy link
Copy Markdown
Contributor

I'm not sure on removing the "build" stage yet. I'm hoping to eventually set up something like Rollup so we can tree-shake and avoid including node_modules in the final bundle, and setting that up would require reinstating that build step and possibly undoing this entire PR later 😓

@JstnMcBrd JstnMcBrd force-pushed the mcb/native-node-ts branch from 8121631 to 1b6fee8 Compare May 22, 2026 08:16
@JstnMcBrd
Copy link
Copy Markdown
Contributor Author

I'm not sure on removing the "build" stage yet. I'm hoping to eventually set up something like Rollup so we can tree-shake and avoid including node_modules in the final bundle, and setting that up would require reinstating that build step and possibly undoing this entire PR later 😓

Hmm. I'm not sure what value minifying, tree-shaking, or bundling would have for a project like this. It's not a web-loaded script, web app, or a library/package where download size+speed matters, and performance gains would likely be marginal.

In comparison, the simplicity + reduced cognitive load + improved devx of simply running the script directly seems very valuable. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants