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

Isolated declarations refactor #115

Merged
merged 3 commits into from
Nov 25, 2023

Conversation

dragomirtitian
Copy link

No description provided.

Copy link

@frigus02 frigus02 left a comment

Choose a reason for hiding this comment

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

Looks like a nice type safety improvement.

@@ -366,6 +415,9 @@ export function transformDeclarations(context: TransformationContext) {
}

function trackReferencedAmbientModule(node: ModuleDeclaration, symbol: Symbol) {
// We forbid references in isolated declarations no need to report any errors on them
if (isolatedDeclarations) return;

Choose a reason for hiding this comment

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

I wonder if some of these early returns should/could be assertions or debug failures. That might make it easier to find errors?

E.g. here it looks like trackReferencedAmbientModule is only called in trackReferencedAmbientModuleFromImport and in checker.ts. The checker tests if the function exists first. Could make it an assertion here, remove the function from the tracker and add either assertion or early return in trackReferencedAmbientModuleFromImport.

All of this could be wrong, though. I have very little context here.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah. Changed it to remove those for function in the symbol tracker.

Copy link

@h-joo h-joo left a comment

Choose a reason for hiding this comment

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

LGTM

},
});
if (isolatedDeclarations) {
const symbolTracker: SymbolTracker = {
Copy link

Choose a reason for hiding this comment

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

Can you move this out of the if statement and assign the specific case to make it more readable the difference?
i.e symbolTracker.moduleResolverHost = host;

Copy link
Author

Choose a reason for hiding this comment

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

I changed a bit to move the symbol tracker creation to a single location. And not create it in isolated declaration mode.

Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>
Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>
Signed-off-by: Titian Cernicova-Dragomir <tcernicovad1@bloomberg.net>
@dragomirtitian dragomirtitian merged commit 2dfc9d1 into isolated-declarations Nov 25, 2023
1 check passed
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.

3 participants