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.

Sign up for GitHub

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

Jump to bottom

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

Open
3 tasks
tylers-username opened this issue Oct 24, 2024 · 3 comments · May be fixed by #6249
Open
3 tasks

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

tylers-username opened this issue Oct 24, 2024 · 3 comments · May be fixed by #6249
Labels
enhancement Improve existing functionality or small fixes

Comments

@tylers-username
Copy link
Contributor

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:

  • Add aliases to typescript & webpack. We can declare the aliases while also not being required to migrate all files to use the aliases at once.
  • Configure eslint to understand these aliases. Done via eslint-import-resolver-typescript
  • **Use declare module '@alias/.... To help each file we migrate to ts to understand how their imported packages work.

The above will help devs to efficiently and iteratively migrate JS files to typescript.
@tylers-username tylers-username added the enhancement Improve existing functionality or small fixes label Oct 24, 2024
@tylers-username tylers-username linked a pull request Oct 24, 2024 that will close this issue
Open
@thornbill
Copy link
Member

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
Copy link
Contributor Author

tylers-username commented Oct 24, 2024 •
edited
Loading

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
Copy link
Member

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. 🤷‍♂️

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement Improve existing functionality or small fixes
Projects
None yet
Milestone
No milestone
2 participants
@thornbill @tylers-username