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

update dependencies #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kontrollanten
Copy link

@kontrollanten kontrollanten commented Dec 22, 2024

  • Update dependencies

Copy link
Owner

@JohnXLivingston JohnXLivingston left a comment

Choose a reason for hiding this comment

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

Thanks for your PR.
Updating ESLint and typescript is more than welcome.

But i have some concerns about some changes you made.

Copy link
Owner

Choose a reason for hiding this comment

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

Why do you change the stylelintrc filetype ? Is there a good reason?
Using a JS file allows comment (if needed). That could help if the configuration become more complexe one day.

"build:server": "npx tsc --build server/tsconfig.json",
"build:client": "node ./scripts/build.js",
"build:server": "npx tsc --build tsconfig.json",
"build:client": "node ./scripts/build.mjs",
Copy link
Owner

Choose a reason for hiding this comment

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

Why have you changed the file type to a mjs?

This is a script, not a module. Is there a good reason that i missed?

"build:styles": "npx sass --no-source-map assets:dist/assets",
"build": "npm-run-all -s clean check:client:tsc -p build:server build:client build:styles",
"lint:script": "npx eslint --ext .ts --ext .js .",
"lint:script": "eslint",
Copy link
Owner

Choose a reason for hiding this comment

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

Why do you removed the npx call?
Using npx ensure that it is using the eslint version in the node_modules directory, and not any globally installed eslint (which could cause some issue if the version is not matching).

"esbuild": "^0.14.36",
"eslint": "^8.21.0",
"eslint-config-standard-with-typescript": "^22.0.0",
"@peertube/peertube-types": "^7.0.0",
Copy link
Owner

Choose a reason for hiding this comment

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

There was a good reason to let "@peertube/peertube-types": "^4.2.2",: in the engine section, I mention that this plugin is compatible with "peertube": ">=4.2.0".
So we must use the Peertube 4.2.x types, to be sure to not use features that are not avaible.
This is explained in the README.

"eslint": "^8.21.0",
"eslint-config-standard-with-typescript": "^22.0.0",
"@peertube/peertube-types": "^7.0.0",
"@tsconfig/node18": "^18.2.4",
Copy link
Owner

Choose a reason for hiding this comment

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

The tsconfig/nodexx dependency should match the node version supported by the supported Peertube version.
This template is meant for Peertube >= 4.2.0, so it should be Node16.

@@ -1,10 +1,13 @@
import type { RegisterServerOptions } from '@peertube/peertube-types'

async function register ({ peertubeHelpers }: RegisterServerOptions): Promise<void> {
await Promise.resolve()
Copy link
Owner

Choose a reason for hiding this comment

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

Why?

peertubeHelpers.logger.info('Hello world')
}

async function unregister (): Promise<void> {}
async function unregister (): Promise<void> {
await Promise.resolve()
Copy link
Owner

Choose a reason for hiding this comment

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

Why?

@@ -1,7 +1,7 @@
{
"extends": "@tsconfig/node16/tsconfig.json",
"extends": "@tsconfig/node18/tsconfig.json",
Copy link
Owner

Choose a reason for hiding this comment

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

See previous comment. Should be related to the supported Peertube version.

"compilerOptions": {
"moduleResolution": "node", // Tell tsc to look in node_modules for modules
"moduleResolution": "node16", // Tell tsc to look in node_modules for modules
Copy link
Owner

Choose a reason for hiding this comment

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

see previous comment.

Comment on lines +17 to +27
"outDir": "./dist/",
"paths": {},
"allowJs": true
},
"include": [
"./**/*"
"./server/**/*",
"**/*.mjs"
],
"exclude": []
"exclude": [
"./client"
]
Copy link
Owner

Choose a reason for hiding this comment

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

Why to you moved the server tsconfig file?
Server and Client configuration were separate. Moving the server tsconfig in the root directory adds confusion (IMHO).

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