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: Allow renderer v4 #443

Merged
merged 3 commits into from
Dec 13, 2024
Merged

fix: Allow renderer v4 #443

merged 3 commits into from
Dec 13, 2024

Conversation

d3xter666
Copy link
Contributor

Currently the linter check only if renderer is explicitly defined with version 2.
There's also version 4 for it. We need to take into account this, so to avoid false positives

@d3xter666 d3xter666 requested a review from a team December 11, 2024 08:50
@@ -346,13 +346,16 @@ export default class SourceFileLinter {
if (node && (ts.isObjectLiteralExpression(node) || ts.isVariableDeclaration(node))) {
const apiVersionNode = findApiVersionNode(node) as ts.PropertyAssignment | ts.NumericLiteral | undefined;

const availableApiVersions = ["2", "4"];
Copy link
Member

Choose a reason for hiding this comment

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

Maybe there'll be a v5 some day which we probably shouldn't report as deprecated?

Copy link
Member

Choose a reason for hiding this comment

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

I think we initially limited this to "2" to be able to complain about "unknown" api versions. The same is now true for "4".
But I get your point how to keep this up-to-date with the runtime capabilities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I was wondering for that, too.
An easy solution would be < 2, but we still don't have 5 and also there isn't (officially) version 3.
So, setting those values would produce false positives. To be honest, I'm not sure what's the renderer algorithm that detects versioning... would it accept values other than 2 and 4 and if yes, what would be the result?

Copy link
Member

Choose a reason for hiding this comment

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

Runtime fact: The runtime defaults a missing apiVersion to 1 and contains checks whether the version === 1 (or !== 1) and another check whether apiVersion === 4.

That being set, any version > = 1 would be fine technically, BUT documentation explicitly defines contracts for versions 1, 2 and 4 only, not for any other version. So no other version should be specified by control developers.

Copy link
Member

Choose a reason for hiding this comment

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

As my reply might not be clear enough: I'm in favour of the current check for 2 or 4 as valid, everything else is not supported in UI5 2.0 (as of now).

@d3xter666 d3xter666 merged commit 2e2d8d2 into main Dec 13, 2024
13 checks passed
@d3xter666 d3xter666 deleted the fix-renderer-version-check branch December 13, 2024 09:47
@openui5bot openui5bot mentioned this pull request Dec 13, 2024
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