jellyfin / jellyfin-web

Web Client for Jellyfin
https://jellyfin.org
GNU General Public License v2.0
2.41k stars 1.28k forks source link

Improvement: Set foundation to improve migrate-to-typescript efficiency #6248

Open tylers-username opened 1 month ago

tylers-username commented 1 month ago

In order to migrate to Typescript efficiently, and more importantly with open source, iteratively, we need to be able to migrate one to a few js files to ts at a time. Until a JS file is migrated to TS, we need something like the following in place to avoid errors in the files that have been migrated to typescript.

declare module 'scripts/settings/webSettings' {
    export function getPlugins(): Promise<string[]>;
}

The issue with the above declaration is that some files refer to ./scripts/, while others refer to scripts/. To support a typescript migration, we should add an alias to webpack and to tsconfig for our base directories in src/ such as:

"@scripts/*": ["scripts/*"],

The @ serves as a standard indicator to future devs that it is an alias or that, at least, it is not a relative path to the current directory (for components requesting from a subdirectory).

tl;dr:

The above will help devs to efficiently and iteratively migrate JS files to typescript.

thornbill commented 1 month ago

The @ serves as a standard indicator

Do you have some references for this? The last time I looked it seemed there was no real standard... ~ and no prefix seemed pretty common. Personally I find using @ a bit confusing since it conflicts with org names for npm packages (see @jellyfin/sdk, @mui/*).

The only reason we haven't enforced the change to not using relative imports really is that it would cause conflicts with the ~100 open PRs. There is a plan to focus on these types of improvements for the 10.11 release though so now is probably the best time to make such a change.

tylers-username commented 1 month ago

The @ serves as a standard indicator

"Standard" is certainly not the right word to use in this case. Coffee-brain and hyping my own ticket probably led to this 😄

I believe the issue with no prefix is that it isn't immediately clear to the developer (someone coming in fresh as i did) where the package lives—there's a need to check: "is it relative to my current directory?"; "why do some use ./components and others use components/ in similar subdirectories?"

The problem with allowing for ./scripts and scripts during the migration period from JS to TS is that declare modules 'scripts' will only work for scripts not ./scripts. I suspect declare modules will be a temporary measure to ease the burden of migrating and should no longer be needed once all files have been migrated to TS.

The only reason we haven't enforced the change to not using relative imports really is that it would cause conflicts with the ~100 open PRs.

I believe the MR takes this into consideration since scripts and ./scripts, components and ./components will continue to work. Only TS files will need to be updated to use import {prefix}scripts IF the imported file is not already typescript.

thornbill commented 1 month ago

I just checked the Bulletproof React docs and they recommend using @/* which would address my concern with the org naming. Since the goal is to move the project structure in that direction, maybe it makes sense to adopt that convention also. :man_shrugging:

https://github.com/alan2207/bulletproof-react/blob/master/docs/project-standards.md#absolute-imports