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

fix: Improve module resolution #440

Merged
merged 1 commit into from
Dec 10, 2024
Merged

fix: Improve module resolution #440

merged 1 commit into from
Dec 10, 2024

Conversation

matz3
Copy link
Member

@matz3 matz3 commented Dec 6, 2024

This solves issues where some detections did not work properly due to
the way dependencies were resolved.

When using relative imports, the module resolution was working, but when
for example importing a control from a test, the complete namespace is
used, but the corresponding JS file was not found.

This change ensures that JS files are resolved and also that the types
for a framework library are loaded when it is linted.

@matz3 matz3 requested a review from a team December 6, 2024 10:40
Copy link
Contributor

@d3xter666 d3xter666 left a comment

Choose a reason for hiding this comment

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

LGTM, but it seems coverage needs to be either improved or decreased a bit

@d3xter666 d3xter666 requested a review from a team December 6, 2024 14:43
@matz3
Copy link
Member Author

matz3 commented Dec 9, 2024

I checked the difference and in SourceFileLinter.ts there is now less code covered in analyzeControlRendererDeclaration, as it takes a different path for the fixed case.

@d3xter666 do you know whether there is still a case that needs the handling in here?

// Check all exports
moduleExportsSymbols?.forEach((exportSymbol) => {
// Note: getAliasedSymbol fails when called with a symbol that isn't an alias
let symbol = exportSymbol;
if (exportSymbol.flags & SymbolFlags.TypeAlias) {
// Export could be a "default", so we need to get the real reference of the export symbol
symbol = this.checker.getAliasedSymbol(exportSymbol);
}
symbol?.getDeclarations()?.forEach((declaration) => {
if (ts.isVariableDeclaration(declaration) && declaration.initializer) {
this.analyzeControlRendererInternals(declaration.initializer);
} else if (ts.isExportAssignment(declaration) && declaration.expression) {
this.analyzeControlRendererInternals(declaration.expression);
}
});
});

@d3xter666
Copy link
Contributor

d3xter666 commented Dec 9, 2024

I checked the difference and in SourceFileLinter.ts there is now less code covered in analyzeControlRendererDeclaration, as it takes a different path for the fixed case.

@d3xter666 do you know whether there is still a case that needs the handling in here?

// Check all exports
moduleExportsSymbols?.forEach((exportSymbol) => {
// Note: getAliasedSymbol fails when called with a symbol that isn't an alias
let symbol = exportSymbol;
if (exportSymbol.flags & SymbolFlags.TypeAlias) {
// Export could be a "default", so we need to get the real reference of the export symbol
symbol = this.checker.getAliasedSymbol(exportSymbol);
}
symbol?.getDeclarations()?.forEach((declaration) => {
if (ts.isVariableDeclaration(declaration) && declaration.initializer) {
this.analyzeControlRendererInternals(declaration.initializer);
} else if (ts.isExportAssignment(declaration) && declaration.expression) {
this.analyzeControlRendererInternals(declaration.expression);
}
});
});

It seems that this test/file test/fixtures/linter/projects/sap.f/src/sap/f/ProductSwitch.js
does not go through the moduleExportsSymbols... loop anymore. It's resolved here:
https://github.com/SAP/ui5-linter/blob/main/src/linter/ui5Types/SourceFileLinter.ts#L286

I believe this is caused by the new namespace resolve logic in here and also the conditional inclusion of the .d.ts files... I'm starting to question whether we need this custom module resolution logic anymore. But I suppose there might be cases where we intentionally might include the .d.ts files and then, we'll definitely need that.

Edit: Just checked it. Removed this section https://github.com/SAP/ui5-linter/blob/main/src/linter/ui5Types/SourceFileLinter.ts#L288-L332 and the tests are green. Also checked the sap.f and sap.m libs. Nothing suspicious there, too. Then it seems to be safe to remove this whole section

@matz3
Copy link
Member Author

matz3 commented Dec 9, 2024

@d3xter666 thanks a lot 👍🏻 Then I'll go ahead with that. We can still revert it in case we encounter some relevant cases again.

@matz3 matz3 force-pushed the fix-module-resolution branch from 7466d37 to f9c805b Compare December 9, 2024 15:41
This solves issues where some detections did not work properly due to
the way dependencies were resolved.

When using relative imports, the module resolution was working, but when
for example importing a control from a test, the complete namespace is
used, but the corresponding JS file was not found.

This change ensures that JS files are resolved and also that the types
for a framework library are loaded when it is linted.
@matz3 matz3 force-pushed the fix-module-resolution branch from f9c805b to 3438e2b Compare December 9, 2024 15:43
@matz3 matz3 requested a review from d3xter666 December 9, 2024 15:43
@matz3 matz3 merged commit ce4eed8 into main Dec 10, 2024
13 checks passed
@matz3 matz3 deleted the fix-module-resolution branch December 10, 2024 08:14
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.

2 participants