feat(Formatter): Detect globals and strings of formatters in bindings (JS/TS) #499
Conversation
56fa7f7 to
64ddc1d
Compare
d11d061 to
6d14323
Compare
6d14323 to
6c9a169
Compare
| { | ||
| column: 48, | ||
| fatal: true, | ||
| line: 5, | ||
| message: 'Value of property \'formatter\' is of type \'string\'.', | ||
| messageDetails: 'Do not use strings for \'formatter\' values in JavaScript and TypeScript. See Best Practices for Developers (https://ui5.sap.com/#/topic/28fcd55b04654977b63dacbee0552712)', | ||
| ruleId: 'parsing-error', | ||
| severity: 2, | ||
| }, |
There was a problem hiding this comment.
@flovogt @codeworrior This snapshot shows the detection of formatter properties that have a value of type string in bindings (regardless if strings or object literals). This is not a deprecation, but a helpful hint for devs, as the runtime cannot resolve this declaration even with a formatter import.
For the time being, I have thrown a parsing error for such detections because I could not find a more suitable ruleId.
What do you think? Should we design a new ruleId for this? e.g.: "no-strings-in-formatter-properties" ? However, this would be very specific and not fit into our naming conventions for ruleIds.
There was a problem hiding this comment.
Please, take a look at my comment here: #499 (comment)
There was a problem hiding this comment.
This snapshot shows the detection of formatter properties that have a value of type string in bindings (regardless if strings or object literals).
I struggle with the "regardless". In binding strings, a formatter string IS supported by the runtime, in binding objects it IS NOT.
Therefore, complaining about a use of strings in a binding object does NOT fall into the category "worked in UI5 1.x, but no longer works in UI5 2.x" and therefore is not in the current focus of the linter. It also does not fall into the category of "no-deprecated-api" as it never worked. It's rather a "unsupported-api-usage" oder "illegal-argument" or whatever.
In binding strings, it's different.
| // Although the formatter property does not look like a global notation, | ||
| // it should still be detected if it is a string: | ||
| const input2 = new Input({ | ||
| value: "{ path: 'invoice>Status', formatter: 'formatter.statusText' }" |
There was a problem hiding this comment.
In this case, it's clear that there cannot be fromatter like this:
"{ path: 'invoice>Status', formatter: formatter.statusText }", (no quotes around formatter.statusText) because the BindingParser/ExpressionParser will throw an exception.
Therefore, the current state is the only possible way to define this.
Why do we need to report this case?
https://github.com/SAP/ui5-linter/pull/499/files#diff-f32c91d608e9b3542dbe158a009067f34d4ccdc04b59ab81039bcb497db55703R80
The only reasonable answer would be that the formatter will not be supported when defining a string binding.
@codeworrior do I miss something here?
There was a problem hiding this comment.
To avoid misunderstandings, we're talking about the following case with quotes around the formatter value and where formatter is the import name of the corresponding module:
sap.ui.define(["some/formatter", ...], function(formatter, ...) {
new Input({
value: "{ path: 'invoice>Status', formatter: 'formatter.statusText' }"
});
});This does NOT work at runtime. The local variable formatter (introduced by the sap.ui.define call) is visible only within the scope of the module factory function. The framework code (which parses the binding string) is in a different scope and does not know this variable nor does it have access to it. So, for the framework code, 'formatter.statusText' looks like any other global name and that's why it should be reported.
There was a problem hiding this comment.
I suggested to add this as a fixture mainly for ourselves, so that we don't think this would be a valid usage of a formatter in a binding string.
There was a problem hiding this comment.
The only reasonable answer would be that the formatter will not be supported when defining a string binding.
If you change this slightly to
The only reasonable recommendation would be that the formatter will no longer be supported in UI5 2.0 when defining a string binding.
then I agree.
There was a problem hiding this comment.
Now, I get it! Thanks a lot!
| { | ||
| column: 48, | ||
| line: 6, | ||
| message: 'Do not use strings for \'formatter\' values in JavaScript and TypeScript.', | ||
| messageDetails: 'See Best Practices for Developers (https://ui5.sap.com/#/topic/28fcd55b04654977b63dacbee0552712)', | ||
| ruleId: 'unsupported-api-usage', | ||
| severity: 2, | ||
| }, |
| { | ||
| column: 11, | ||
| line: 6, | ||
| message: 'Access of global variable \'ui5\' (ui5.walkthrough.model.formatter.statusText)', | ||
| messageDetails: 'Do not use global variables to access UI5 modules or APIs. See Best Practices for Developers (https://ui5.sap.com/#/topic/28fcd55b04654977b63dacbee0552712)', | ||
| ruleId: 'no-globals', | ||
| severity: 2, | ||
| }, | ||
| { | ||
| column: 11, | ||
| line: 9, | ||
| message: 'Access of global variable \'ui5\' (ui5.walkthrough.model.formatter.statusText)', | ||
| messageDetails: 'Do not use global variables to access UI5 modules or APIs. See Best Practices for Developers (https://ui5.sap.com/#/topic/28fcd55b04654977b63dacbee0552712)', | ||
| ruleId: 'no-globals', | ||
| severity: 2, | ||
| }, | ||
| { | ||
| column: 11, | ||
| line: 15, | ||
| message: 'Access of global variable \'formatter\' (formatter.statusText)', | ||
| messageDetails: 'Do not use global variables to access UI5 modules or APIs. See Best Practices for Developers (https://ui5.sap.com/#/topic/28fcd55b04654977b63dacbee0552712)', | ||
| ruleId: 'no-globals', | ||
| severity: 2, | ||
| }, |
There was a problem hiding this comment.
Lines 58-81:
These findings are the result of detecting strings for formatter values inside a Binding String (previously: "parsing-error", now: "no-globals").
| [ | ||
| { | ||
| coverageInfo: [], | ||
| errorCount: 5, |
There was a problem hiding this comment.
When looking at the file I would expect 3 findings. To me it looks like there are two duplicate findings.
There was a problem hiding this comment.
As aligned with @codeworrior and approved by @flovogt, we want to detect two cases here:
- If the
formatterproperty is of type string inside a BindingString - If a global notation of the formatter property was used (here: "ui5.walkthrough...")
Since both of these cases apply for this file, there are two findings each.
Nonetheless, we've decided to use the same linter message.
There was a problem hiding this comment.
Both cases should cause a finding, but as they originate from the same place in the code, there is only one finding to be solved. So only one should be reported.
There was a problem hiding this comment.
Maybe we should adjust the linter messages to better emphasize the difference. But both cases should still be reported and not just one imo.
JIRA: CPOUI5FOUNDATION-1005