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

Manual update some dependency #124

Closed
4 tasks
yanguoyu opened this issue Apr 6, 2023 · 13 comments
Closed
4 tasks

Manual update some dependency #124

yanguoyu opened this issue Apr 6, 2023 · 13 comments
Assignees
Labels
enhancement New feature or request

Comments

@yanguoyu
Copy link
Author

yanguoyu commented May 9, 2023

For updating typescript to V5, it's blocked by ttypescript. Because of this cevek/ttypescript#140 (comment), and the ttypescript seems not active. And someone gives a solution nonara/ts-patch#93 (comment) with ts-patch, but this repository owner may not have much time to update it nonara/ts-patch#93 (comment).

So I have four solutions to fix it:

  1. use roll-up or babel to resolve the alias problem, they are stable but it seems a waste and heavy to use them.
  2. Replace ttypescript with ts-patch, I think it can work with typescript V5 at least.
  3. Wrapper require function to try to get the module from dist directory.
  4. Output the tsc file to dist/node_modules, but it will change the output directory struct with input. It may introduce other problems.

I suggest using the second solution, although it's maybe updating not timely, we only use this to transform the path, I think it's enough. But after I use the transform plugin with ts-patch, the compiler progress crashed with out of memory. Then I find another package tsc-alias to alias the path after tsc. It works.

Another thing I am concerned about is whether the package tsc-alias will be safe in the future. Because it will change the outputs after tsc. Should we alias the required path by ourselves? If so, I think 3 or 4 is better, or we can remove tsconfig's baseUrl @Keith-CY

@Keith-CY
Copy link
Member

Keith-CY commented May 9, 2023

For updating typescript to V5, it's blocked by ttypescript. Because of this cevek/ttypescript#140 (comment), and the ttypescript seems not active. And someone gives a solution nonara/ts-patch#93 (comment) with ts-patch, but this repository owner may not have much time to update it nonara/ts-patch#93 (comment).

So I have four solutions to fix it:

  1. use roll-up or babel to resolve the alias problem, they are stable but it seems a waste and heavy to use them.
  2. Replace ttypescript with ts-patch, I think it can work with typescript V5 at least.
  3. Wrapper require function to try to get the module from dist directory.
  4. Output the tsc file to dist/node_modules, but it will change the output directory struct with input. It may introduce other problems.

I suggest using the second solution, although it's maybe updating not timely, we only use this to transform the path, I think it's enough. But after I use the transform plugin with ts-patch, the compiler progress crashed with out of memory. Then I find another package tsc-alias to alias the path after tsc. It works.

Another thing I am concerned about is whether the package tsc-alias will be safe in the future. Because it will change the outputs after tsc. Should we alias the required path by ourselves? If so, I think 3 or 4 is better, or we can remove tsconfig's baseUrl @Keith-CY

Does paths in tsconfig.json meet the request?

Ref:

@yanguoyu
Copy link
Author

yanguoyu commented May 9, 2023

Does paths in tsconfig.json meet the request?

Ref:

microsoft/TypeScript#10866
microsoft/TypeScript#10866 (comment)
It's a known issue, tsc will not change the module path, and they think path mapping reflects the behavior of an external resolution scheme.

@WhiteMinds
Copy link

I think we can remove the import aliases because I believe the cost it brings is less than the benefits.

Benefits of using import aliases:

  1. Improves readability (although sometimes it's not necessarily the case, I often mistake utils for a library)
  2. No need to update import codes when moving files

Costs of using import aliases:

  1. Increases workflow complexity, any workflow that needs to handle the source code must configure the alias, such as tsc, vite, webpack, IDE (VSCode), etc., which also means that it reduces the available ecosystem.
  2. Increases cognitive burden (sometimes the source of the imported file needs to be clicked to confirm)

I actually don't think the benefit of not updating the import code is necessary because moving will always result in a new commit, and recording IDE's automatic update import records in it will not affect anything. IDEs hardly make mistakes with this kind of relative path change.

Regarding the readability of imports, I think it is not very important. Often, we don't care about import codes, and we only need IDE's automatic import and eslint's automatic sorting of import orders.

Therefore, taking all factors into account, I think the cost we pay is greater than our benefits. We can continue to use it if there were no problems before, but since we are currently blocked by import aliases, we can consider removing them.

@yanguoyu @Keith-CY What do you think?

@Keith-CY
Copy link
Member

Keith-CY commented May 9, 2023

I think we can remove the import aliases because I believe the cost it brings is less than the benefits.

Benefits of using import aliases:

  1. Improves readability (although sometimes it's not necessarily the case, I often mistake utils for a library)
  2. No need to update import codes when moving files

Costs of using import aliases:

  1. Increases workflow complexity, any workflow that needs to handle the source code must configure the alias, such as tsc, vite, webpack, IDE (VSCode), etc., which also means that it reduces the available ecosystem.
  2. Increases cognitive burden (sometimes the source of the imported file needs to be clicked to confirm)

I actually don't think the benefit of not updating the import code is necessary because moving will always result in a new commit, and recording IDE's automatic update import records in it will not affect anything. IDEs hardly make mistakes with this kind of relative path change.

Regarding the readability of imports, I think it is not very important. Often, we don't care about import codes, and we only need IDE's automatic import and eslint's automatic sorting of import orders.

Therefore, taking all factors into account, I think the cost we pay is greater than our benefits. We can continue to use it if there were no problems before, but since we are currently blocked by import aliases, we can consider removing them.

@yanguoyu @Keith-CY What do you think?

I also value the readability of code because most time I read code from GitHub which is not so smart to resolve imports. Especially alias, it may be resolved to an external package occasionally.

@WhiteMinds
Copy link

I also value the readability of code because most time I read code from GitHub which is not so smart to resolve imports. Especially alias, it may be resolved to an external package occasionally.

Yes, that's exactly what I wanted to say about the importance of following standards. By following standards, we can better utilize various ecosystems.
Online IDEs that support importing relative paths are definitely more common than those that support importing aliases.

@yanguoyu
Copy link
Author

yanguoyu commented May 9, 2023

I also value the readability of code because most time I read code from GitHub which is not so smart to resolve imports. Especially alias, it may be resolved to an external package occasionally.

Yes, that's exactly what I wanted to say about the importance of following standards. By following standards, we can better utilize various ecosystems. Online IDEs that support importing relative paths are definitely more common than those that support importing aliases.

I also think it's better to remove the import aliases.

@yanguoyu
Copy link
Author

@WhiteMinds
Copy link

nervosnetwork/neuron#2660

Afterwards, it is probably necessary to provide PRs for modifications to those two large branches(newui / lightclient).

@yanguoyu
Copy link
Author

nervosnetwork/neuron#2660

Afterwards, it is probably necessary to provide PRs for modifications to those two large branches(newui / lightclient).

We will rebase the develop branch regularly after some commits are added to develop branch.

@alexsupa597
Copy link

@WhiteMinds
Copy link

From version 0.2 to 0.3, TypeORM introduced significant changes to concepts and APIs. The APIs we currently use, such as createConnection, getConnectionOptions, and getConnection, have been deprecated.

Migrating to this version may require a considerable amount of time to adapt to the new abstract concepts. Therefore, I suggest creating a separate issue specifically for upgrading TypeORM from version 0.2 to 0.3.

For now, I have only completed a PR to upgrade to the latest version of 0.2 from our current version: nervosnetwork/neuron#2680

@alexsupa597
Copy link

alexsupa597 commented May 31, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

No branches or pull requests

7 participants