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

[experiment] direct imports #51590

Draft
wants to merge 76 commits into
base: main
Choose a base branch
from

Conversation

a-tarasyuk
Copy link
Contributor

@a-tarasyuk a-tarasyuk commented Nov 18, 2022

/** @todo */

  • enable Debug in the scanner/semver modules
  • enable Debug in the core

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Nov 18, 2022
@typescript-bot
Copy link
Collaborator

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

@jakebailey
Copy link
Member

Very exciting that this works! I have yet to review it and take a look at what it does in the bundle, but you basically did all of the hard work for us...

@a-tarasyuk
Copy link
Contributor Author

There is still an open question with Debug that causes circular imports...

@jakebailey
Copy link
Member

There is still an open question with Debug that causes circular imports...

Can you remind me again what breaks? Though there are some circular things, I don't know that any should crash at this point, besides one scanner thing which can be fixed by moving a helper to another file.

Solving circular imports entirely is a further PR just because that will involve a lot of code moving, but eliminating internal _namespaces imports is one step that lets us start running proper tooling to analyze these cycles.

@a-tarasyuk
Copy link
Contributor Author

a-tarasyuk commented Jan 6, 2023

Can you remind me again what breaks?

Currently, there are approximately 10 _circular dependencies, however, core <-> debug is the circular dependency that
breaks the build, need to investigate this dependency, and other dependencies that import core, and debug. We need to revise the module hierarchy - core is the top module, but it depends on debug, which depends on core, utilities (depends on core and debug), etc. :).

compiler-deps

@jakebailey
Copy link
Member

With 5.0 coming out soon, I'd like to try and relook at this to get it in for 5.1 (especially as some amount of this is a prereq for me to test out TS-as-ESM); one thing I'm immediately unsure of is all of the extra utility files being created, especially checkerUtilities.ts; are these strictly required to make the code work? Could some be moved to other files instead, or kept in the checker?

I know that there is some code that should be shifted out of the scanner (or something), but, if they're being created solely to fix cycles, I'm wondering if we'd be better off doing that at a second step to minimize the diff a little.

I'm also planning on sending another partial change that will make this PR smaller, namely, trying something to get rid of most of the _namespaces imports as possible, the ones we can do early.

@a-tarasyuk
Copy link
Contributor Author

a-tarasyuk commented Mar 13, 2023

especially checkerUtilities.ts; are these strictly required to make the code work? Could some be moved to other files instead, or kept in the checker?

(minor) Moving the types to checkerUtilities is necessary to avoid importing the checker into Debug, which causes additional circular imports.

(major) Some files (convertToAsyncFunction.ts, moveToNewFile.ts, symbolWalker.ts, etc.) depend on the getNodeId/getSymbolId utilities, which are not part of TypeChecker. Importing the checker (which includes a lot of other dependencies) into these files causes a crash.

I know that there is some code that should be shifted out of the scanner (or something), but, if they're being created solely to fix cycles

If we are talking about the scanner, some files use utilities (especially files that depend on core/debug/compiler/utilities) from it, which causes circular imports and crashes.

I'm also planning on sending another partial change that will make this PR smaller, namely, trying something to get rid of most of the _namespaces imports as possible, the ones we can do early.

Sounds promising :). Maybe we need to try breaking it up into smaller pieces, however, I'm not sure how granular they should be. Maybe we need to split file by file, however, that adds a lot of new PR and as I recall some cases can't be refactored without touching many files.

@jakebailey
Copy link
Member

getNodeId and getSymbolId can definitely be moved to utilities or something and be fine; I think I'd prefer the most minimal change possible for this.

@a-tarasyuk
Copy link
Contributor Author

Ok, I'll try to move these helpers to utilities. What about types? I think it would be better to avoid using checker in debug.

@jakebailey
Copy link
Member

I would think they can be moved to types.ts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants