Skip to content

feat: Autofix Core API (complex cases) #675

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
Open
wants to merge 69 commits into
base: main
Choose a base branch
from
Open

feat: Autofix Core API (complex cases) #675

wants to merge 69 commits into from
+906 −29

Conversation

d3xter666
Copy link
Contributor

@d3xter666 d3xter666 commented May 28, 2025 •
edited
Loading

JIRA: CPOUI5FOUNDATION-991

Enhances:
#671
#675

Enables getLibraryResourceBundle migration by checking whether sLibrary (the first argument) is an actual SAPUI5 library. The check is performed as follows:

  • check for library existence in TS definitions for SAPUI5
  • Check project's libs in manifest.json
  • If the current project is a library

Reolves partially: #619

matz3
matz3 reviewed May 28, 2025
View reviewed changes
src/linter/ui5Types/fixHints/CoreFixHintsGenerator.ts Outdated
const potentialLibNamespace = node.text;

// Check libs from the manifest
const manifest = JSON.parse(this.manifestContent ?? "{}") as SAPJSONSchemaForWebApplicationManifestFile;
Copy link
Member

Choose a reason for hiding this comment

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

Can't we just re-use the existing logic that extracts manifest.json information instead of doing it again here?
(see

ui5-linter/src/linter/lintWorkspace.ts

Line 173 in 7b1e6dd

async function getLibraryDependenciesFromManifest(workspace: AbstractAdapter, virBasePath: string | undefined) {
)

We could change this to a broader scope, like getInformationFromManifest, and then returning an object with the relevant details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just split the existing solution into separate PRs and now I'll do the refactorings

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry for the premature review 😄

src/linter/ui5Types/fixHints/CoreFixHintsGenerator.ts Outdated

if (manifestLibs[potentialLibNamespace]) {
return true;
} else if (manifest?.["sap.app"]?.id === potentialLibNamespace) {
Copy link
Member

Choose a reason for hiding this comment

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

This check should only be done when the project is a library.

@d3xter666 d3xter666 marked this pull request as draft May 28, 2025 07:00
@d3xter666 d3xter666 force-pushed the autofix-deprecated-core-apis branch 2 times, most recently from 0c07e32 to cb85a25 Compare May 28, 2025 07:46
@d3xter666 d3xter666 force-pushed the autofix-determine-library-resource branch from 56ebba1 to 66098d1 Compare May 28, 2025 08:41
This was referenced May 28, 2025
Merged
Merged
@d3xter666 d3xter666 force-pushed the autofix-deprecated-core-apis branch from f67bfba to d4eed0f Compare May 28, 2025 11:20
@d3xter666 d3xter666 force-pushed the autofix-determine-library-resource branch from 08844d2 to 4ad1d98 Compare May 28, 2025 11:36
@d3xter666 d3xter666 force-pushed the autofix-deprecated-core-apis branch 2 times, most recently from f033503 to 0b46e18 Compare May 29, 2025 07:12
@d3xter666 d3xter666 force-pushed the autofix-determine-library-resource branch 2 times, most recently from 18ec577 to 5015376 Compare May 29, 2025 08:32
@d3xter666 d3xter666 changed the title feat: Determine library resource for autofixer feat: Autofix Core API (complex cases) May 29, 2025
@d3xter666 d3xter666 force-pushed the autofix-deprecated-core-apis branch 2 times, most recently from 3360082 to c815258 Compare May 30, 2025 16:07
@d3xter666 d3xter666 force-pushed the autofix-determine-library-resource branch from bcf4001 to c0ed35a Compare May 30, 2025 16:21
d3xter666 added a commit that referenced this pull request Jun 2, 2025
@d3xter666 @maxreichmann
feat(autofix): Fix basic deprecated Core APIs (#671)
857a03e 
JIRA: CPOUI5FOUNDATION-991

This is the first (base) chunk of several PRs related to the Core APIs
autofix.
All of the other PRs would build on top of it. Here we migrate the
trivial cases where replacements are 1:1

Resolves partially: #619

Follow up PRs:
#639
#675

---------

Co-authored-by: Max Reichmann <max.reichmann@sap.com>
@d3xter666 d3xter666 force-pushed the autofix-deprecated-core-apis branch from c815258 to 1eafe45 Compare June 2, 2025 11:22
d3xter666 added a commit that referenced this pull request Jun 5, 2025
@maxreichmann @d3xter666
feat(autofix): Fix additional deprecated Core APIs (#639)
af2cb9b 
JIRA: CPOUI5FOUNDATION-991

Enhances #671 by enabling more
complex autofixes where migration depends on the provided arguments.

Resolves partially: #619

Follow up PRs:
#675

---------

Co-authored-by: Yavor Ivanov <yavor.ivanov@sap.com>
Base automatically changed from autofix-deprecated-core-apis to main June 5, 2025 12:08
@d3xter666
Copy link
Contributor Author

Will be merged after: #662

@d3xter666 d3xter666 force-pushed the autofix-determine-library-resource branch 2 times, most recently from 9467ff9 to c17665e Compare June 17, 2025 10:27
@d3xter666 d3xter666 marked this pull request as ready for review June 17, 2025 11:38
@d3xter666 d3xter666 requested a review from a team June 17, 2025 11:38
d3xter666 and others added 24 commits June 18, 2025 09:50
@d3xter666
test: Remove not migrated test cases
da15493
@d3xter666
test: Update snapshots
ca417e0
@maxreichmann @d3xter666
feat: Autofix Core APIs (base)
2a58c10
@d3xter666
feat: Determine library resource
1ccd102
@d3xter666
fix: Correctly provide manifest for autofixer
c6da20d
@d3xter666
fix: Manifest content
5e2c39b
@d3xter666
fix: Merge conflicts
5b59478
@d3xter666
fix: Resolve merge conflicts
6f92e16
@d3xter666
fix: Resolve merge conflicts
c28ceab
@d3xter666
test: Update snapshots
ae21451
@d3xter666
refactor: Tests
31e4eea
@d3xter666
refactor: Align code with the major refactoring
630c22e
@d3xter666
refactor: Migrate attachIntervalTimer
2398310
@d3xter666
refactor: Enable migration of loadLibrary
3daad88
@d3xter666
refactor: Enable createComponent
c47932f
@d3xter666
refactor: Enable getLibraryResourceBundle scenarios
96eca13
@d3xter666
refactor: Cleanups
8bc1308
@d3xter666
test: Enable scenarios
88c5e9c
@d3xter666
fix: Merge issues
dd15e5e
@d3xter666
test: Update snapshots
a964ce7
@d3xter666
refactor: Check libraries in manifest
45499b9
@d3xter666
test: Update snapshots
792b9b5
@d3xter666
refactor: Enable lib checks for getLibraryResourceBundle
2590d7f
@d3xter666
fix: Merge conflicts
6e4f54c
@d3xter666 d3xter666 force-pushed the autofix-determine-library-resource branch from 3fff6d0 to 6e4f54c Compare June 18, 2025 06:58
RandomByte
RandomByte requested changes Jun 18, 2025
View reviewed changes
src/linter/ui5Types/fix/collections/sapUiCoreFixes.ts
}
return asyncOption;
} else if (arg2?.kind === SyntaxKind.TrueKeyword) {
if (ts.isStringLiteralLike(arg1)) {
Copy link
Member

Choose a reason for hiding this comment

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

What if it's not a string but an identifier? Shouldn't that be fine as well?

Copy link
Member

Choose a reason for hiding this comment

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

IMO, for the props in the allow list, any value is acceptable. Even if the library name is concatenated or when the url is calculated using e.g. sap.ui.require.toUrl() or whatever.

Only for properties where we have to check for a certain value (e.g. async === true) it makes sense to limit the values to something that we can evaluate.

src/linter/ui5Types/fix/collections/sapUiCoreFixes.ts

if (ts.isPropertyAssignment(node) && ts.isIdentifier(node.name)) {
const name = node.name.text;
if (allowlistProps.includes(name) && ts.isStringLiteralLike(node.initializer)) {
Copy link
Member

@RandomByte RandomByte Jun 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Similar to my other comment, I guess we could also handle identifiers here?

d3xter666 and others added 3 commits June 19, 2025 10:16
@d3xter666 @RandomByte
refactor: Cleanup code
bee72d9 
Co-authored-by: Merlin Beutlberger <m.beutlberger@sap.com>
@d3xter666 @RandomByte
refactor: Improve loadLibrary fixture
4ef88de 
Co-authored-by: Merlin Beutlberger <m.beutlberger@sap.com>
@d3xter666 @RandomByte
refactor: Improve loadLibrary replacements
22580b2 
Co-authored-by: Merlin Beutlberger <m.beutlberger@sap.com>
@d3xter666 d3xter666 force-pushed the autofix-determine-library-resource branch from bde2a18 to 22580b2 Compare June 19, 2025 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matz3 matz3 matz3 left review comments

@codeworrior codeworrior codeworrior left review comments

@RandomByte RandomByte RandomByte requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@d3xter666 @matz3 @RandomByte @codeworrior @maxreichmann