removed tsc-alias

[kurtisassad]

Link to Issue

Closes: #6362

Description of Changes

• Remove tsc alias

Test Plan

• Run build of commonwealth via yarn build && yarn bundle && yarn start-external-webpack and ensured it ran correctly. (Side note, this doesn't work because it runs the .ts file instead of the .js)
• Run yarn build && cd packages/commonwealth && NODE_ENV=production NODE_OPTIONS=--max-old-space-size=$(../../scripts/get-max-old-space-size.sh) node --enable-source-maps ./build/server.js and ensured it ran correctly.
• Run build of discord listener and consumer, made sure that it ran correctly
• Run build of snapshot-listener, made sure that it ran correctly via node ./build/index.js

(dev experience)

• Added a 'server/...' type import on the server side. Ensured that it threw an error at static typecheck time.

[Rotorsoft]

@kurtisassad I was expecting this PR to have a bunch of import renames and removing the "*" path in commonwealth/tsconfig.json

 "paths": {
      "@hicommonwealth/*": ["../../libs/*/src"],
      "*": ["./*", "shared/*", "client/scripts/*"]
    }

The purpose of tsc-alias is to replace the "*" paths following the tsc build step. It was used to reference folders located outside the commonwealth root directory, however, after the refactoring saga, all references are now contained within the root directory, and we can solve it with relative paths. This is not a critical issue though, and we can postpone if too much work involved.

[kurtisassad]

@Rotorsoft It seems that we actually didn't need tsc-alias as the backend builds fine without it. We had never used that type of * alias on the backend (It is only used on the frontend, but webpack handles it, not tsc-alias).

I see what you are saying about the aliases on the backend though. These can be fixed in a subsequent PR in order to separate the concerns.

[timolegros]

We shouldn't remove tsc-alias if we don't first remove "*": ["./*", "shared/*", "client/scripts/*"] from paths. The current implementation breaks on deployment since paths like /server/... or /shared/... will pass type-checks + build but fail when executed. Additionally, even if we refactor existing instances of such imports, the paths property in the tsconfig allows for new imports using such patterns which would again break when deployed.

To replicate the above error you need to run server.js not server.ts (see Procfile).

[kurtisassad]

@timolegros Seems a little strange, since running the server.js works for me. I will try running it on staging

Ah nvm, was running start-external-webpack, which doesn't run the exact production build since it still runs server.ts, not the js variant

[timolegros]

As mentioned in my previous review, removing the tsc-alias dependent imports does make the build work but merging this is dangerous since future tsc-alias dependent imports are still allowed. As such we could accidentally deploy a broken build without realizing it because we don't run server.js in testing/CI.

[kurtisassad]

@timolegros I added in a tsconfig for the server to override this so that it throws an error if we use this pattern on the server.

The issue from a full refactor is that the client uses this pattern a lot, it will be a huge refactor. Anyways with these minimal changes the tsc-alias is no longer needed, because webpack handles the recursive imports on the client side.

[timolegros]

This (check-types) would use the packages/commonwealth/tsconfig.json config so it would pass with the bad imports no? Likely need to temporarily have 2 tsc --noEmit checks here -> one for the client and one for the server.

[kurtisassad]

Yeah so this might be a problem. Basically we don't get any build errors from adding these imports, but we do get editor visual errors:

The only issue with having a separate check-types is that tsc won't actually work because the folder that you are type checking are not allowed to have imports outside the root (In this case we have server importing modules from other packages like @hicommonwealth/core). So I am not sure the solution for this one. Maybe its better just to settle for the editor errors.

[timolegros]

@kurtisassad per convo we should definitely not settle for editor errors. I'm not exactly sure what the solution is here but we should hop in a huddle with @Rotorsoft and figure this out this week.

[timolegros]

@kurtisassad I created an import from shared/ on the server and yarn check-types-server did no throw an error. Also, we may want to modify the sanity.sh script to include type-checking the server once it is fixed.

[Rotorsoft]

@timolegros are you considering the type checking discussed here in #7407 ?

[rbennettcw]

Some conflicts

[ForestMars]

Setting back to draft pending conficts.

[jnaviask]

@kurtisassad what happened with this PR?