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.

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

fix: Allow renderer v4 #443

Merged
merged 3 commits into from
Dec 13, 2024
Merged

fix: Allow renderer v4 #443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d8e7e5
fix: Allow renderer v4
d3xter666 Dec 11, 2024
bab4939
fix: Eslint issues
d3xter666 Dec 11, 2024
ca54787
test: Add test case with invalid render version
d3xter666 Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/linter/ui5Types/SourceFileLinter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).

let nodeToHighlight: ts.PropertyAssignment | ts.PropertyDeclaration |
ts.VariableDeclaration | ts.ObjectLiteralExpression |
ts.NumericLiteral | undefined = undefined;
if (!apiVersionNode) { // No 'apiVersion' property
nodeToHighlight = node;
} else if ((ts.isNumericLiteral(apiVersionNode) && apiVersionNode.getText() !== "2") ||
(ts.isPropertyAssignment(apiVersionNode) && apiVersionNode.initializer.getText() !== "2")) {
} else if ((ts.isNumericLiteral(apiVersionNode) &&
!availableApiVersions.includes(apiVersionNode.getText())) ||
(ts.isPropertyAssignment(apiVersionNode) &&
!availableApiVersions.includes(apiVersionNode.initializer.getText()))) {
// String value would be "\"2\""
nodeToHighlight = apiVersionNode;
}
Expand Down
9 changes: 9 additions & 0 deletions test/fixtures/linter/rules/renderer/ControlRendererDeclaration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
const { default: Renderer } = require("sap/ui/core/Renderer");

sap.ui.define(["sap/ui/core/Control", "sap/m/Button"], function(Control, Button) {
const Example1 = Control.extend("sap.ui.demo.linter.controls.Example1", {
metadata: {},
Expand All @@ -22,4 +24,11 @@ sap.ui.define(["sap/ui/core/Control", "sap/m/Button"], function(Control, Button)
metadata: {},
// Missing renderer declaration (deprecated)
});
const Example6 = Button.extend("sap.ui.demo.linter.controls.Example6", {
metadata: {},
renderer: {
apiVersion: 3, // Invalid API version
render: function(oRm, oControl) {}
}
});
});
39 changes: 23 additions & 16 deletions test/lib/linter/rules/snapshots/renderer.ts.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ Generated by [AVA](https://avajs.dev).
},
{
coverageInfo: [],
errorCount: 3,
errorCount: 2,
fatalErrorCount: 0,
filePath: 'ControlRendererDeclaration_negative.js',
messages: [
Expand All @@ -441,14 +441,6 @@ Generated by [AVA](https://avajs.dev).
ruleId: 'no-deprecated-api',
severity: 2,
},
{
column: 4,
line: 52,
message: 'Use of deprecated renderer detected. Define explicitly the {apiVersion: 2} parameter in the renderer object',
messageDetails: '"Renderer Object (https://ui5.sap.com/#/topic/c9ab34570cc14ea5ab72a6d1a4a03e3f)",',
ruleId: 'no-deprecated-api',
severity: 2,
},
{
column: 2,
line: 4,
Expand All @@ -461,46 +453,61 @@ Generated by [AVA](https://avajs.dev).
warningCount: 0,
},
{
coverageInfo: [],
errorCount: 5,
coverageInfo: [
{
category: 1,
column: 31,
line: 1,
message: 'Unable to analyze this method call because the type of identifier in "require("sap/ui/core/Renderer")"" could not be determined',
},
],
errorCount: 6,
fatalErrorCount: 0,
filePath: 'ControlRendererDeclaration.js',
messages: [
{
column: 4,
line: 30,
message: 'Use of deprecated renderer detected. Define explicitly the {apiVersion: 2} parameter in the renderer object',
messageDetails: '"Renderer Object (https://ui5.sap.com/#/topic/c9ab34570cc14ea5ab72a6d1a4a03e3f)",',
ruleId: 'no-deprecated-api',
severity: 2,
},
{
column: 13,
line: 5,
line: 7,
message: 'Deprecated declaration of renderer \'sap.ui.demo.linter.controls.Example1Renderer\' for control \'Example1\'',
messageDetails: 'Defining the \'renderer\' for control \'Example1\' by its name may lead to synchronous loading of the \'sap/ui/demo/linter/controls/Example1Renderer\' module. Import the \'sap/ui/demo/linter/controls/Example1Renderer\' module and assign it to the \'renderer\' property.',
ruleId: 'no-deprecated-control-renderer-declaration',
severity: 2,
},
{
column: 13,
line: 10,
line: 12,
message: 'Deprecated declaration of renderer \'sap.ui.demo.linter.controls.Example2Renderer\' for control \'Example2\'',
messageDetails: 'Defining the \'renderer\' for control \'Example2\' by its name may lead to synchronous loading of the \'sap/ui/demo/linter/controls/Example2Renderer\' module. Import the \'sap/ui/demo/linter/controls/Example2Renderer\' module and assign it to the \'renderer\' property.',
ruleId: 'no-deprecated-control-renderer-declaration',
severity: 2,
},
{
column: 15,
line: 15,
line: 17,
message: 'Deprecated declaration of renderer for control \'Example3\'',
messageDetails: 'Defining the \'renderer\' for control \'Example3\' by its name may lead to synchronous loading of the renderer module. Import the renderer module and assign it to the \'renderer\' property.',
ruleId: 'no-deprecated-control-renderer-declaration',
severity: 2,
},
{
column: 19,
line: 17,
line: 19,
message: 'Control \'Example4\' is missing a renderer declaration',
messageDetails: 'Not defining a \'renderer\' for control \'Example4\' may lead to synchronous loading of the \'Example4Renderer\' module. If no renderer exists, set \'renderer: null\'. Otherwise, either import the renderer module and assign it to the \'renderer\' property or implement the renderer inline.',
ruleId: 'no-deprecated-control-renderer-declaration',
severity: 2,
},
{
column: 19,
line: 21,
line: 23,
message: 'Control \'Example5\' is missing a renderer declaration',
messageDetails: 'Not defining a \'renderer\' for control \'Example5\' may lead to synchronous loading of the \'Example5Renderer\' module. If no renderer exists, set \'renderer: null\'. Otherwise, either import the renderer module and assign it to the \'renderer\' property or implement the renderer inline.',
ruleId: 'no-deprecated-control-renderer-declaration',
Expand Down
Binary file modified test/lib/linter/rules/snapshots/renderer.ts.snap
View file
Open in desktop
Binary file not shown.
Loading