Skip to content
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

Added ts node approach #6112

Merged
merged 39 commits into from
Feb 5, 2024
Merged

Added ts node approach #6112

merged 39 commits into from
Feb 5, 2024

Conversation

SamTV12345
Copy link
Member

No description provided.

@SamTV12345 SamTV12345 changed the title Fixed determining file extension. Added ts node approach Jan 20, 2024
src/package.json Outdated
"test-container": "mocha --timeout 5000 tests/container/specs/api"
"test": "mocha --require ts-node/register --timeout 120000 --recursive tests/backend/specs ../node_modules/ep_*/static/tests/backend/specs",
"test-container": "mocha --require ts-node/register --timeout 5000 tests/container/specs/api",
"dev": "ts-node node/server.ts",
Copy link

@AugustinMauroy AugustinMauroy Jan 20, 2024

Choose a reason for hiding this comment

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

you can run ts file whit tsx like that node --import tsx ./file.ts

Ref:

Copy link
Member Author

Choose a reason for hiding this comment

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

Is tsx better than ts-node?

Copy link

@AugustinMauroy AugustinMauroy Jan 20, 2024

Choose a reason for hiding this comment

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

with ts-node you call a script that runs the typescriopt script, whereas with tsx in loader node it's node that runs the script directly. Normally tsx is supposed to perform better, but I'm not 100% sure.

EDIT: i have found this link

@@ -0,0 +1,109 @@
{
"compilerOptions": {
/* Visit https://aka.ms/tsconfig to read more about this file */

Choose a reason for hiding this comment

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

I thinks we can remove all commented key to have something more clean. What do you thinks ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion. I did that.

Choose a reason for hiding this comment

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

👍

src/package.json Outdated
@@ -108,7 +108,8 @@
"test": "mocha --import=tsx --timeout 120000 --recursive tests/backend/specs ../node_modules/ep_*/static/tests/backend/specs",
"test-container": "mocha --import=tsx --timeout 5000 tests/container/specs/api",
"dev": "node --import tsx node/server.ts",
"prod": "node --import tsx node/server.ts"
"prod": "node --import tsx node/server.ts",
"ts-check": "tsc --noEmit --watch"

Choose a reason for hiding this comment

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

Suggested change
"ts-check": "tsc --noEmit --watch"
"ts-check": "tsc --noEmit",
"ts-check:watch": "tsc --noEmit --watch"

it's allow us to use ts-check on CI and ts-check:watch for dev. What do you think ?

Comment on lines 1 to 2
import {Readable} from "node:stream";
import {ChildProcess} from "node:child_process";

Choose a reason for hiding this comment

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

If you import something as type use this syntax:

Suggested change
import {Readable} from "node:stream";
import {ChildProcess} from "node:child_process";
import type {Readable} from "node:stream";
import type {ChildProcess} from "node:child_process";

this avoids confusion for junior devs.

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I fixed that.

@AugustinMauroy
Copy link

is it a good idea to call the directory model? I'd go for types or maybe you've also got people using @types but I'm not a big fan because it creates confusion with DefinitelyTyped

@SamTV12345
Copy link
Member Author

is it a good idea to call the directory model? I'd go for types or maybe you've also got people using @types but I'm not a big fan because it creates confusion with DefinitelyTyped

Maybe not. I renamed it to types.

@@ -116,7 +116,8 @@ jobs:
-
name: Install Cypress
run: cd etherpad && cd src && npm install cypress --legacy-peer-deps
- name: Install npm@6
-
name: Install npm@6
run: npm install npm@6 -g

Choose a reason for hiding this comment

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

why install npm because it's already ship with node

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that Etherpad is one of the first projects that started to use node and npm. The only problem is that to run Etherpad you require this old npm version. More recent versions of npm will upgrade the package-lock.json file which breaks the Etherpad installation. There is a branch available that tries to do the npm update but there is unfortunately currently no one looking at it.

Choose a reason for hiding this comment

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

I see. In short, the infrastructure is "old". We can start with typescript.
I'm willing to open up an issue for the roadmap of how to modernise the infrastructure if you (the ether contributors) are interested.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that would be great. Thanks. I guess the best starting point is currently Typescript. After that current npm version and then move to ES instead of CJS.

@@ -1,7 +1,9 @@
'use strict';

import {Socket} from "node:net";
import {MapArrayType} from "../types/MapType";

const _ = require('underscore');

Choose a reason for hiding this comment

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

normally this pr allow us to migrate the back-end to esm ?

You should test that :

Suggested change
const _ = require('underscore');
import _ from 'underscore';

Copy link
Member Author

Choose a reason for hiding this comment

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

Does not work unfortunately. That throws an error in NodeJS. It only works for types because they are ignored by jsx.

Choose a reason for hiding this comment

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

Could you give exact error. Because it's should work.
Unless the type in the package.json is set to commonjs.

Copy link
Member Author

Choose a reason for hiding this comment

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

When I go to express.ts and change the import of SecretRotater to import {SecretRotater} from '../security/SecretRotater' it says TypeError import_SecretRotater.SecretRotater is not a constructor

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh. So everytime I use import I also have to make sure that the script to be imported uses the ES syntax. Otherwise I get weird errors.

Choose a reason for hiding this comment

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

Ok, I thinks it's because the source file/package isn't use ES syntax .

@@ -1,7 +1,9 @@
'use strict';

import {Socket} from "node:net";
import {MapArrayType} from "../types/MapType";

Choose a reason for hiding this comment

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

don't forget to add type keyword 😄

Suggested change
import {MapArrayType} from "../types/MapType";
import type {MapArrayType} from "../types/MapType";

@@ -160,10 +165,10 @@ exports.restartServer = async () => {
}

// Measure response time
app.use((req, res, next) => {
app.use((req:any, res:any, next:Function) => {

Choose a reason for hiding this comment

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

What if req doesn't have any specifique type ? And what if we type req: Request

REF

@@ -0,0 +1,10 @@
'use strict';

Choose a reason for hiding this comment

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

use strict is still needed with typescript ?

@SamTV12345 SamTV12345 merged commit ead3c0e into develop Feb 5, 2024
31 of 41 checks passed
@SamTV12345 SamTV12345 deleted the feature/typescript-ts-node branch February 5, 2024 20:13
@AugustinMauroy
Copy link

why all CI didn't pass

@SamTV12345
Copy link
Member Author

why all CI didn't pass

Because there is a major outage for the ubuntu repository for libreoffice. Thus the pipelines fail.

@AugustinMauroy
Copy link

ok

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.

2 participants