Skip to content

Issue/2593 Upgrade Typescript Project to Use Project References#2641

Closed
t83714 wants to merge 40 commits intomasterfrom
issue/2593
Closed

Issue/2593 Upgrade Typescript Project to Use Project References#2641
t83714 wants to merge 40 commits intomasterfrom
issue/2593

Conversation

@t83714
Copy link
Contributor

@t83714 t83714 commented Nov 29, 2019

What this PR does

Fixes #2593

Upgraded the followings:

  • typescript: to 3.7.2
  • ts-node: to 8.5.2
  • tsconfig-paths: to 3.9.0
  • webpack: to 4.41.2
    • for support of new version ts-loader which support project references
    • It's for connectors only (to build browser version)
    • preview-map, web-client are still on their current versions
  • added tsconfig-paths-webpack-plugin for better project references support as well

Changes Summary:

  • Update tsconfig-global.json:
    • Added new options introduced by new typescript version
    • Merged with current settings
    • "composite": true: enable compile by projects
    • Turn off "noEmitOnError":
      • In project compile mode, new typescript version will stop emitting by default (i.e. when this option off) when meeting any errors. However, if set this option to true, ts will stop emit for dependent projects and leave you a no emit error.
  • Code Changes:
    • replace any @magda/typescript-common/dist with magda-typescript-common/src
    • replace any @magda/minion-framework/dist with magda-minion-framework/src
    • --esModuleInterop is turned on by default for new version TS. so replace import commonJs moulde import * as foo from "foo" with import foo from foo (it will work even the module has no default export). see here
      • It doesn't mean you should never use import * as --- es6 module with no default export can still import all export in that way.
      • It's only about commonJS modules (esp. module export a function eg. nock --- it's against ECMAScript spec as a namespace import can't be a callable)
    • Other code changes due to strict type checking & bugs fixes:
      • magda-typescript-common/src/express/status.ts:
        • Defines a function on one if branch is not allowed.
        • Fix a logic problem that miss checking nextState is undefined
      • magda-typescript-common/src/tenant-api/AuthorizedTenantClient.ts:
        • getTenants: make exception not all muted so it matches other code.

Limitation on typescript & Other consideration:

Typescript compiler won't allow you to have two different compile-time module path & runtime module path, which make it difficult to deploy compiled commonJS module code.

e.g. if you put import x from "magda-typescipt-common/src"; in your code, the tsc build result will output require("magda-typescipt-common/src") in the built code.

And typescript won't allow you to map it to a runtime path (e.g. @magda/typescript-common/dist or magda-typescipt-common/dist like the feature that babel, webpack & rollup all provides). I think this is not gonna be changed in short-term as Microsoft team believe mapping the runtime module path alters import statement's behaviour. Thus it's against typescript's design goal 7. See here.

project references feature won't help this either.

There are solutions like tsconfig-paths available. But it's for ts-node (works well for our test cases with project references) and our runtime env in docker is quite different (can't be figured out from tsconfig.json).

Thus, my current solution is to use Babel's babel-plugin-module-resolver module to replace module alias in TS build result - The current build script is tsc -b && babel dist -d dist;

  • I've deployed the built result to https://issue-2593.dev.magda.io/

@AlexGilleran @kring
You probably won't like it. Thus, create a draft PR to collect feedback, ideas and decide where we go 😄

Other solutions I can think of:

  • Use bundle tools to compile the build. e.g. ts-loader works well with the webpack for all connectors setups
  • Write a simple bootstrap code (like tsconfig-paths/register) to override require. We will need to alter the way we run our code. e.g. node -r ourModuleAlias.register dist/index.js. Will need to update our helm charts.
  • Re-organise magda-typescript-common & magda-minion-framework:
    • Make all module import through magda-typescript-common/ or magda-minion-framework/ (no src)
      • e.g. import addJwtSecretFromEnvVar from "magda-typescript-common/session/addJwtSecretFromEnvVar";
    • Build as AMD modules and output as a signle file.
    • Use project reference prepend option to include magda-typescript-common & magda-minion-framework in build file. e.g. "references": [{ "path": "../magda-typescript-common", "prepend": true }] will include AMD module build output at beginning of the built file.
    • However, node doesn't support AMD so we still need a runtime loader.

@AlexGilleran @kring

Please let me know your idea~

Test Site:

https://issue-2593.dev.magda.io/

Checklist

  • There are unit tests to verify my changes are correct
  • I've updated CHANGES.md with what I changed.
  • I've linked this PR to an issue in ZenHub (core dev team only)

t83714 and others added 30 commits November 25, 2019 12:49
increase the cache key
- remove noEmitOnError from global as it's not appropriate for project reference setup
- add tsbuildinfo to gitignore
@t83714 t83714 requested review from AlexGilleran and kring December 2, 2019 00:48
@AlexGilleran
Copy link
Contributor

I think I'm OK with this as long as it solves @kring 's problems 👍

@t83714
Copy link
Contributor Author

t83714 commented Dec 8, 2019

@AlexGilleran
Re-create a new PR from this branch to remove the no. of the moving pieces required.
New PR here:
#2650

@t83714 t83714 closed this Dec 8, 2019
@sajidanower23 sajidanower23 deleted the issue/2593 branch January 21, 2020 02:05
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.

Use TypeScript Project References to make the build faster and less fragile

2 participants