-
Notifications
You must be signed in to change notification settings - Fork 106
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
[ASI-837] Allow typescript + javascript interoperability in content-node #2816
Conversation
…o dm-fix-typescript-requires
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great catch! looks like we need to update tests to be ts-compatible, maybe have to add ts-mocha? have a look in libs package.json for an example
@dylanjeffers yes! i'm working on that now. thanks for paving the way 🙂 |
…o dm-fix-typescript-requires
@@ -1071,8 +1073,7 @@ describe('test Polling Tracks with real files', function () { | |||
.set('User-Id', userId) | |||
.send(trackMetadata) | |||
.expect(200) | |||
trackMetadataMultihash = trackMetadataResp.body.data.metadataMultihash | |||
trackMetadataFileUUID = trackMetadataResp.body.data.metadataFileUUID | |||
const trackMetadataFileUUID = trackMetadataResp.body.data.metadataFileUUID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these were never actually defined as variables and it was now throwing an error with ts-node
@@ -3,7 +3,7 @@ | |||
"baseUrl": "./src", | |||
"pretty": true, | |||
"target": "es2020", | |||
"module": "es2020", | |||
"module": "commonjs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the actual fix, confirmed by dylan with link to official tsconfig files https://github.com/tsconfig/bases/blob/main/bases/node16-strictest.combined.json#L10
@@ -27,7 +27,7 @@ const { | |||
} = require('../middlewares') | |||
|
|||
const { getCID } = require('./files') | |||
const { decode } = require('../hashids.js') | |||
const { decode } = require('../hashids') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have to omit the extension because locally it's reading the ts file but in the prod image it's using a transpiled version of js. the transpiled version should be 1-1 to the ts so functionally the same
@dylanjeffers tests pass now, i also took it out to one of our stage content nodes and tested the new function that was moved to typescript so the transpile to the static build/ folder also seems to work. if you wanted to take one more look before merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Description
Currently in content-node typescript was hard to integrate because we couldn't have something like
index.ts -> file.js -> new_file.ts
where -> means requires()to incrementally switch to TS.
However, with this option changed we can now have the above dependency graph.
Tests
Tested this locally and took it out to stage creator node 4 to make sure it compiles, comes up and serves requests.
I converted the hashids.js file to hashids.ts and added a log to tracks.js to print out the encoded id for 6. This demonstrates that ts can require a js file which can require another ts file with full interoperability
tracks.js
logs
How will this change be monitored? Are there sufficient logs?
Services won't start if there's a bug here. Health check would immediately identify this